-
Notifications
You must be signed in to change notification settings - Fork 389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Breaking: Default to traitlets based Build class #1521
Conversation
85395d8
to
5980df8
Compare
ee1425b
to
ef059f2
Compare
ef059f2
to
635fb4e
Compare
5990312
to
79731ac
Compare
37f4612
to
137260a
Compare
137260a
to
316b2da
Compare
4bb0fd5
to
b0080fd
Compare
f40a37b
to
4200584
Compare
4fa1465
to
0ac8ec0
Compare
0ac8ec0
to
597e43d
Compare
BuildExecutor.identifier for BinderHub /version endpoint
Values have been passed as env-vars instead for quite a while now https://github.com/jupyterhub/binderhub/blob/2ba938bf20201ff864b3a6c473f66f30fbef92bc/binderhub/binderspawner_mixin.py#L103-L110
597e43d
to
8a48dac
Compare
This is ready for review! |
# Construct a Builder so that we can extract parameters such as the | ||
# configuration or the version string to pass to /version and /health handlers | ||
example_builder = self.build_class(parent=self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not confident about this situation, but wondering if it would make practical sense to work with class state on the build class itself instead of state object instance state for this information.
If so, we wouldn't have to create a dummy object, but instead just read the relevant information from the class.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this relates to identifier
now being made a configurable traitlet. If we go for class state, the identifier
field should probably not be a traitlet - but it doesn't seem like something that makes sense for users to configure themselves, but rather like something for the creator of the buildexecutor class to declare.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's based on a traitlets value, which AFAIK can't be static. I've renamed the variable where it's used to make it clearer, see e38480a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it makes sense for this to be configurable, lets go with this as it is! Can you imagine a plausible situation when this could make sense to configure?
# is this relevant/sensible to support?
c.KubernetesBuildExecutor.builder_info = <something>
I'm currently understanding builder_info
as something inherent to the builder class providing it, like a __version__
field, which I don't think should be configurable. So I guess there are two parts to my question about this:
- Should this be made configurable by using a trait with
.config(True)
- Should this be class state or object state (where I also think the use of class state prohibits the use of a trait to store it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For KubernetesBuildExecutor
the default value includes build_image
which is a user-configurable traitlet, so unless traitlets properties can be static builder_info
can't be a static class property, e.g.:
{
"builder_info": {
"build_image": "quay.io/manics/repo2docker:2023-01-02-12-10-arm64"
},
"binderhub": "0.2.0+1243.gf5f8c33.dirty",
"builder": "quay.io/manics/repo2docker:2023-01-02-12-10-arm64"
}
Compare with the current output: https://mybinder.org/versions
For config=True/False
..... debatable. /versions
is a public endpoint, including on authenticated BinderHubs, so one use could be to deliberately hide the builder_info
. Not sure if that's useful..... what do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, well i think its not worth further thinking if we didnt come up with a clean alternative strategy by now.
PR LGTM ready for merge in my mind!
Thank you for amazing efforts in binderhub Simon!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @manics!!! ❤️ 🎉 |
jupyterhub/binderhub#1521 Merge pull request #1521 from manics/build-traitlets-2
This is a follow-up to #1518 that switches the default build class from
Build
to the new Traitlets basedKubernetesBuildExecutor
class.