-
Notifications
You must be signed in to change notification settings - Fork 323
Add user-defined builder image in image spec #3207
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
Add user-defined builder image in image spec #3207
Conversation
Signed-off-by: Nelson Chen <[email protected]>
Signed-off-by: Nelson Chen <[email protected]>
Signed-off-by: Nelson Chen <[email protected]>
Signed-off-by: Nelson Chen <[email protected]>
Signed-off-by: Nelson Chen <[email protected]>
…r_image_in_image_spec
Code Review Agent Run #7a262cActionable Suggestions - 1
Review Details
|
Changelist by BitoThis pull request implements the following key changes.
|
UV_IMAGE=image_spec.builder_config["uv_image"], | ||
MICROMAMBA_IMAGE=image_spec.builder_config["micromamba_image"], |
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.
The PR adds two new variables to the DOCKER_FILE_TEMPLATE.substitute()
call: UV_IMAGE
and MICROMAMBA_IMAGE
. These variables are used in the Dockerfile template on lines 96-97 to specify the base images for the multi-stage build. Consider adding validation to ensure these keys exist in image_spec.builder_config
to prevent potential KeyError exceptions.
Code suggestion
Check the AI-generated fix before applying
UV_IMAGE=image_spec.builder_config["uv_image"], | |
MICROMAMBA_IMAGE=image_spec.builder_config["micromamba_image"], | |
UV_IMAGE=image_spec.builder_config.get( | |
"uv_image", "ghcr.io/astral-sh/uv:0.5.1"), | |
MICROMAMBA_IMAGE=image_spec.builder_config.get( | |
"micromamba_image", "mambaorg/micromamba:2.0.3-debian12-slim"), |
Code Review Run #7a262c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Signed-off-by: Nelson Chen <[email protected]>
Signed-off-by: Nelson Chen <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3207 +/- ##
==========================================
- Coverage 81.95% 77.84% -4.11%
==========================================
Files 346 214 -132
Lines 27852 22356 -5496
Branches 2920 2921 +1
==========================================
- Hits 22826 17404 -5422
+ Misses 4191 4103 -88
- Partials 835 849 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code Review Agent Run #ac0eb1Actionable Suggestions - 0Review Details
|
flytekit/image_spec/image_spec.py
Outdated
if self.builder_config is None: | ||
self.builder_config = { | ||
"uv_image": "ghcr.io/astral-sh/uv:0.5.1", | ||
"micromamba_image": "mambaorg/micromamba:2.0.3-debian12-slim", | ||
} |
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.
Since this is specific to the image builder, I think the default should be set in flytekit/image_spec/default_builder.py
itself.
For example, this configuration does not mean anything for the envd image builder)
Signed-off-by: Nelson Chen <[email protected]>
Signed-off-by: Nelson Chen <[email protected]>
Signed-off-by: Nelson Chen <[email protected]>
Code Review Agent Run #fb7615Actionable Suggestions - 0Review Details
|
uv_image = "ghcr.io/astral-sh/uv:0.5.1" | ||
micromamba_image = "mambaorg/micromamba:2.0.3-debian12-slim" | ||
if image_spec.builder_config is not None: | ||
if image_spec.builder_config.get("uv_image") is not None: | ||
uv_image = image_spec.builder_config.get("uv_image") | ||
|
||
if image_spec.builder_config.get("micromamba_image") is not None: | ||
micromamba_image = image_spec.builder_config.get("micromamba_image") |
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 nit:
uv_image = "ghcr.io/astral-sh/uv:0.5.1" | |
micromamba_image = "mambaorg/micromamba:2.0.3-debian12-slim" | |
if image_spec.builder_config is not None: | |
if image_spec.builder_config.get("uv_image") is not None: | |
uv_image = image_spec.builder_config.get("uv_image") | |
if image_spec.builder_config.get("micromamba_image") is not None: | |
micromamba_image = image_spec.builder_config.get("micromamba_image") | |
if image_spec.builder_config is not None: | |
uv_image = image_spec.builder_config.get("uv_image", DEFAULT_UV_IMAGE) | |
micromamba_image = image_spec.builder_config.get("micromamba_image", DEFAULT_MICROMAMBA_IMAGE) |
where the defaults are constants at the module level:
DEFAULT_UV_IMAGE = "ghcr.io/astral-sh/uv:0.5.1"
DEFAULT_MICROMAMBA_IMAGE = "mambaorg/micromamba:2.0.3-debian12-slim"
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 for the advice! It is more readable.
Signed-off-by: Nelson Chen <[email protected]>
Signed-off-by: Nelson Chen <[email protected]>
Code Review Agent Run #66e237Actionable Suggestions - 0Review Details
|
Signed-off-by: Nelson Chen <[email protected]>
…/flytekit into add_builder_image_in_image_spec
Code Review Agent Run #24a266Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Signed-off-by: Nelson Chen <[email protected]>
Code Review Agent Run Status
|
Tracking issue
Why are the changes needed?
According to the discussion in #3180, It seems more reasonable to add this configuration in image_spec rather than config.yaml.
What changes were proposed in this pull request?
Add an argument in image_spec to let users specify uv_image and micromamba_image in their own registry.
How was this patch tested?
This patch can be tested by add builder_config into image_spec.
if we don't specify the image, the default images are ghcr.io/astral-sh/uv:0.5.1 and mambaorg/micromamba:2.0.3-debian12-slim.
Setup process
Run a task using the image_spec above, and you can find out that the uv_image and micromamba_image are both using the image you specified.
Screenshots
Check all the applicable boxes
Related PRs
#3180
Docs link
Summary by Bito
This PR introduces configurable builder images via the builder_config parameter, replacing hardcoded references with defaults in the image_spec module. The changes support more flexible configuration, allowing custom registry images for uv and micromamba images as discussed in issue #3180. It also updates the Dockerfile template, enhances GeoPandas encoder, and improves Slurm connectors.Unit tests added: True
Estimated effort to review (1-5, lower is better): 2