-
Notifications
You must be signed in to change notification settings - Fork 459
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
feat(sdk): support volume mount in tune API #2508
base: master
Are you sure you want to change the base?
feat(sdk): support volume mount in tune API #2508
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
a2d2c5f
to
898d047
Compare
898d047
to
222020d
Compare
a0fbd20
to
ec38767
Compare
@truc0 Amazing! Thanks for doing this. And I'm sorry for the late reply. I'll review this PR in this week. cc @kubeflow/wg-automl-leads @helenxie-bit @mahdikhashan |
@truc0 thank you - would you please add unit tests and e2e for your changes - unit tests in this path: I'll review the code and functionality after it. thanks for your time. |
Sure, I will add it soon |
Signed-off-by: truc0 <[email protected]>
ec38767
to
a005199
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.
@truc0 I'm so sorry for the late response. Here are my initial comments for you.
Please also take a look at this PR if you are available @kubeflow/wg-automl-leads @helenxie-bit @mahdikhashan @Doris-xm
TuneStoragePerTrialType = TypedDict( | ||
"TuneStoragePerTrial", | ||
{ | ||
"volume": Union[client.V1Volume, Dict[str, Any]], | ||
"mount_path": Union[str, client.V1VolumeMount], | ||
}, | ||
) | ||
|
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.
@truc0 That's an amazing optimization from the perspective of Data Scientists. Could you also help with the review of kubeflow/trainer#2449 (comment), which might be simiar to this scenario?👀
@@ -198,6 +206,7 @@ def tune( | |||
env_per_trial: Optional[ | |||
Union[Dict[str, str], List[Union[client.V1EnvVar, client.V1EnvFromSource]]] | |||
] = None, | |||
storage_per_trial: Optional[List[TuneStoragePerTrialType]] = None, |
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 thinking about the name convention here. We already have storage_config
parameters here and have similar functionality. Do we have any ideas dealing with them? Or combine them together?
- volume: Either a kubernetes.client.V1Volume object or a dictionary | ||
containing volume configuration with required fields: | ||
- name: Name of the volume | ||
- type: One of "pvc", "secret", "config_map", or "empty_dir" |
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.
- type: One of "pvc", "secret", "config_map", or "empty_dir" | |
- type: One of "pvc", "secret", "configmap", or "empty_dir" |
Usually, we'll name it as configmap
instead of config_map
:)
- For config_map: config_map_name, items (optional), default_mode | ||
(optional), optional (optional) |
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.
Same naming convention issue.
elif volume_type == "config_map": | ||
volume = client.V1Volume( | ||
name=volume_name, | ||
config_map=client.V1ConfigMapVolumeSource( | ||
name=storage["volume"].get("config_map_name"), |
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.
Same naming convention issue.
/ok-to-test |
Could you please fix the pre-commit error? Now, you can rerun the CI test with |
thanks @Electronic-Waste , i already have requested some changes here: #2508 (comment) cc: @truc0 |
What this PR does / why we need it:
As discussed on #2247 , providing a clean and simple way for specifying volume mount in
tune
API of katib Python SDK will enhance develop experience.This PR adds
storage_per_trial
argument toKatibClient.tune()
method.Design
The
storage_per_trial
argument is designed to have the following type:To simplify the usage, there are some enhancements:
storage_per_trial
to:Union[TuneStoragePerTrial, List[TuneStoragePerTrial]]
dict
instead of aclient.V1Volume
object when the storage config is simple:mount_path
with astr
instead ofclient.V1VolumeMount
Example Usage
Full Example
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #2247
Checklist:
storage_per_trial