Image spec builder options#3233
Conversation
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
Code Review Agent Run #83541fActionable Suggestions - 1
Additional Suggestions - 7
Review 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 |
2b8f16d to
e84b0dc
Compare
Changelist by BitoThis pull request implements the following key changes.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3233 +/- ##
===========================================
+ Coverage 52.32% 79.19% +26.87%
===========================================
Files 215 215
Lines 22391 22408 +17
Branches 2927 2934 +7
===========================================
+ Hits 11716 17747 +6031
+ Misses 9988 3817 -6171
- Partials 687 844 +157 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
24ce8ae to
7dc2335
Compare
| pip_index: Specify the custom pip index url | ||
| pip_extra_index_url: Specify one or more pip index urls as a list | ||
| pip_secret_mounts: Specify a list of tuples to mount secret for pip install. Each tuple should contain the path to | ||
| Attributes: |
There was a problem hiding this comment.
Updated all this Doc to fix linter
There was a problem hiding this comment.
cc @ppiegaze - your upcoming flytekit PR renders these changes moot right? Along with the monodocs build? I've been out of the loop on the doc building stuff for a while, do you know what the plan for that is? or maybe @cosmicBboy?
There was a problem hiding this comment.
For context, I was doing my best here to get tests passing under the assumption that I was upholding the current standards. I am not aware of ongoing changes and potential CICD inconsistencies.
thomasjpfan
left a comment
There was a problem hiding this comment.
Minor comments, otherwise LGTM
Code Review Agent Run #413df0Actionable 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 |
17157d2 to
3efb7de
Compare
|
LGTM |
Code Review Agent Run #30b4efActionable Suggestions - 0Additional Suggestions - 1
Review 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 |
wild-endeavor
left a comment
There was a problem hiding this comment.
just a couple questions. This will not be used at all by the default builder correct?
| pip_index: Specify the custom pip index url | ||
| pip_extra_index_url: Specify one or more pip index urls as a list | ||
| pip_secret_mounts: Specify a list of tuples to mount secret for pip install. Each tuple should contain the path to | ||
| Attributes: |
There was a problem hiding this comment.
cc @ppiegaze - your upcoming flytekit PR renders these changes moot right? Along with the monodocs build? I've been out of the loop on the doc building stuff for a while, do you know what the plan for that is? or maybe @cosmicBboy?
| return None | ||
|
|
||
| def _update_attribute(self, attr_name: str, values: Union[str, List[str]]) -> "ImageSpec": | ||
| def _update_attribute(self, attr_name: str, values: Union[str, List[str], Dict[str, Any]]) -> "ImageSpec": |
There was a problem hiding this comment.
should the type hint here (and in the class definition) really be dict[str, Any] rather than just dict[str, str]? What are the other types users might add?
There was a problem hiding this comment.
I can see a future, however don't have a specific example, where int and potentially List[str] be used but it seems builder dependent. Happy to constrain to Dict[str, str] and we could later decide to move toward Dict[str, Any] if we find compelling future cases.
Correct, not be used by the default builder. That being said, it is not currently restricted for future use at this time |
Provide the ability to specify image `builder` specific options per Image Spec. Signed-off-by: Mike Hotan <mike@union.ai>
Signed-off-by: Mike Hotan <mike@union.ai>
3efb7de to
d6b6494
Compare
Code Review Agent Run #c907a9Actionable Suggestions - 0Additional Suggestions - 1
Review 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 |
|
Congrats on merging your first pull request! 🎉 |
* Image spec builder options Provide the ability to specify image `builder` specific options per Image Spec. Signed-off-by: Mike Hotan <mike@union.ai> * Add builder_options validation Signed-off-by: Mike Hotan <mike@union.ai> * updates Signed-off-by: Mike Hotan <mike@union.ai> --------- Signed-off-by: Mike Hotan <mike@union.ai>
* Image spec builder options Provide the ability to specify image `builder` specific options per Image Spec. Signed-off-by: Mike Hotan <mike@union.ai> * Add builder_options validation Signed-off-by: Mike Hotan <mike@union.ai> * updates Signed-off-by: Mike Hotan <mike@union.ai> --------- Signed-off-by: Mike Hotan <mike@union.ai>
* Adds ImageSpec.with_runtime_packages (#3231) * Adds ImageSpec.with_dev_dependencies Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fix Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Add tests for noop builder Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Use runtime_packages Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Add docs abount how to use runtime packages Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Less diffs Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fix formatting Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fix docstring Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Dix docstring Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Let pip default to user by itself to be more compatible Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> --------- Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Image spec builder options (#3233) * Image spec builder options Provide the ability to specify image `builder` specific options per Image Spec. Signed-off-by: Mike Hotan <mike@union.ai> * Add builder_options validation Signed-off-by: Mike Hotan <mike@union.ai> * updates Signed-off-by: Mike Hotan <mike@union.ai> --------- Signed-off-by: Mike Hotan <mike@union.ai> --------- Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> Signed-off-by: Mike Hotan <mike@union.ai> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
* Image spec builder options Provide the ability to specify image `builder` specific options per Image Spec. Signed-off-by: Mike Hotan <mike@union.ai> * Add builder_options validation Signed-off-by: Mike Hotan <mike@union.ai> * updates Signed-off-by: Mike Hotan <mike@union.ai> --------- Signed-off-by: Mike Hotan <mike@union.ai> Signed-off-by: Atharva <atharvakulkarni172003@gmail.com>
Why are the changes needed?
This PR provides the ability to specify image
builderspecific options to Image Spec.In a more realistic example, Union image builder is looking the ability to configure and set registry credentials through this attribute.
What changes were proposed in this pull request?
This PR adds:
imagespec.with_builder_optionsto additively accumulate builder_optionsHow was this patch tested?
Additional unit tests
Summary by Bito
This PR enhances ImageSpec functionality by enabling dictionary-based builder-specific options with type validation and clearer error messages. It improves error handling in image_spec.py and updates docstring formatting to meet pydoclint standards, reducing discrepancies between documented and actual attributes. New test cases verify proper handling of invalid builder_options inputs, strengthening the robustness, reliability and clarity of the image specification API.Unit tests added: True
Estimated effort to review (1-5, lower is better): 1