-
Notifications
You must be signed in to change notification settings - Fork 88
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
Helm chart images: conda removed -> pip only, usage disclaimer added, minimized Dockerfile complexity #533
Helm chart images: conda removed -> pip only, usage disclaimer added, minimized Dockerfile complexity #533
Conversation
b45fbf5
to
725d4e5
Compare
725d4e5
to
a3c588f
Compare
a3c588f
to
453842d
Compare
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.
Minor thoughts only. I wonder if you can compare the resultant pip-based image size with conda - is it a significant savings?
One reason for including conda (/mamba) is that users might expect it to be something they can call from within workers. Or maybe not.
Thanks a lot for your review @martindurant!!!! I'm headed away so I can't go through it fully now so I'll return to it tomorrow/later. Cheers! |
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.
Just a few small nits, otherwise this looks good to me. Thanks Erik!
Co-authored-by: Jim Crist-Harif <jcristharif@gmail.com>
Co-authored-by: Jim Crist-Harif <jcristharif@gmail.com>
@martindurant @jcrist thanks for your review! I've responded to the review comments! I know that this and #538 will have merge conflicts so I'll need to do some work as soon as one is merged and I'm quite anxious to keep moving. Btw, I recommend using the "files changed" section to see the discussion as GitHub doesn't seem to make various comments show up in a sensible manner otherwise. I added 7eca0c6 to include OCI standardized labels in all the Dockerfiles we maintain. |
No savings in size on the image that I think matters to compare, dask-gateway-server. The build time of both images are significantly reduced though.
Yes maybe, but I see it as a feature of this PR to make a clear break of the potential expectations someone may have on this image - it is to be used for testing purposes. |
Big win on one, and slightly worse on the other. Interesting! The big win is a good argument to go ahead. |
Hey @martindurant, thanks for your attention! This PR blocks work to fix merge conflicts in #538 and would help having merged to continue development in #514 where I have a not yet understood test failure and I hope to rule out it's related to the old Dockerfiles. Please ping me for any followup questions or and I'll work to address them with haste! |
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.
All good, happy with those conversations. Feel free to pick your favourite wording around the "production" image
Co-authored-by: Martin Durant <martindurant@users.noreply.github.com>
Thank you @martindurant!! I updated |
👍 |
Discussion for this change was made in #524 (comment).
Change summary
conda
andmamba
is no longer installed as part of these images, something I'm quite strongly opinionated about at the moment. If the Helm chart was a separate project that could build these images after dask-gateway and dask-gateway-server has been released and packaged to conda-forge, I'd think differently as the complexity comes mainly from that. I think this change is so problematic as there is now a more explicit expectation.api
pod andcontroller
pod isn't something users have overridden historically I thinkCloses #524
Closes #489 - by declaring that the user is to make their own images for scheduler/workers if they have specific needs.
Closes #432 - by going for plain pip instead.