Skip to content
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

Merged
merged 11 commits into from Jan 9, 2023

Conversation

manics
Copy link
Member

@manics manics commented Aug 3, 2022

This is a follow-up to #1518 that switches the default build class from Build to the new Traitlets based KubernetesBuildExecutor class.

@manics manics force-pushed the build-traitlets-2 branch 5 times, most recently from 85395d8 to 5980df8 Compare August 15, 2022 19:13
@manics manics force-pushed the build-traitlets-2 branch 2 times, most recently from ee1425b to ef059f2 Compare August 21, 2022 22:37
@manics manics force-pushed the build-traitlets-2 branch 2 times, most recently from 5990312 to 79731ac Compare December 16, 2022 22:48
@manics manics force-pushed the build-traitlets-2 branch 2 times, most recently from 4bb0fd5 to b0080fd Compare December 17, 2022 00:01
@manics manics force-pushed the build-traitlets-2 branch 4 times, most recently from f40a37b to 4200584 Compare December 24, 2022 23:50
@manics manics force-pushed the build-traitlets-2 branch 3 times, most recently from 4fa1465 to 0ac8ec0 Compare December 29, 2022 16:52
@manics manics mentioned this pull request Dec 29, 2022
@manics manics changed the title Draft: Breaking: Default to traitlets based Build class Breaking: Default to traitlets based Build class Dec 30, 2022
@manics manics marked this pull request as ready for review December 30, 2022 23:44
@manics
Copy link
Member Author

manics commented Dec 30, 2022

This is ready for review!

Comment on lines +884 to +886
# 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)
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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:

  1. Should this be made configurable by using a trait with .config(True)
  2. 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)

Copy link
Member Author

@manics manics Jan 3, 2023

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?

Copy link
Member

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!!!

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for amazing work @manics!!!

@minrk, since this is a breaking change, do you mind having a look also?

@consideRatio consideRatio merged commit e27f63f into jupyterhub:main Jan 9, 2023
@consideRatio
Copy link
Member

Thanks @manics!!! ❤️ 🎉

consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Jan 9, 2023
@manics manics deleted the build-traitlets-2 branch January 9, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants