[SDK] Add env & env_from in client tune
#2235
Conversation
google-oss-prow
bot
requested review from
andreyvelich,
anencore94 and
tenzen-y
shipengcheng1230
force-pushed
the
add_env_settings
branch
from
c4d0be9
to
9a02b0e
Compare
| @@ -2,3 +2,4 @@ grpcio>=1.41.1 | |||
| protobuf>=3.19.5, <=3.20.3 | |||
| googleapis-common-protos==1.53.0 | |||
| optuna>=3.0.0 | |||
| cmaes>=0.10.0 | |||
There was a problem hiding this comment.
I don't think that we need this since optuna already specify the cmaes version:
There was a problem hiding this comment.
It seems the cmaes is listed as an optional depedency. And in the GitHub CI it says
ModuleNotFoundError: No module named 'cmaes'
I think a change like from optuna>=3.0.0 to optuna[cmaes]>=3.0.0 is probably needed for the test to pass.
There was a problem hiding this comment.
Oh, I see. Thanks for the clarifying. Let's specify cmaes>=0.10.0.
There was a problem hiding this comment.
@shipengcheng1230 @tenzen-y Did optuna recently change their dependancies to make cmaes optional ?
There was a problem hiding this comment.
This commit moves it: optuna/optuna@78159b5
| env: Environment variable(s) to be attached to each trial container. | ||
| env_from: Source(s) of environment variables to be populated in each trial container. |
There was a problem hiding this comment.
Can we mention the types like resources_per_trial?
There was a problem hiding this comment.
Great catch! I have added additional type descriptions of these two (4661b80)
shipengcheng1230
force-pushed
the
add_env_settings
branch
from
9a02b0e
to
4661b80
Compare
shipengcheng1230
force-pushed
the
add_env_settings
branch
from
4661b80
to
3c5ad4a
Compare
| env: Union[Dict[str, str], List[V1EnvVar], None] = None, | ||
| env_from: Union[V1EnvFromSource, List[V1EnvFromSource], None] = None, |
There was a problem hiding this comment.
Should we keep the name associated with Trials ? E.g. user could misunderstand this as environment for Algorithm Service.
E.g. name it as trials_env or env_per_trial similar to resources_per_trial.
Also, I think we can allow to set Dict[str, str], List[V1EnvVar], or List[V1EnvFromSource] in this parameter.
There was a problem hiding this comment.
I like env_per_trial which is more descriptive and consistent with resources_per_trial!
Also, I think we can allow to set Dict[str, str], List[V1EnvVar], or List[V1EnvFromSource] in this parameter.
In this case, we are merging the two env and env_from into a single env_per_trial (or trials_env), am I right? (I also love the simplicity here for users)
There was a problem hiding this comment.
Yeah, I think so @shipengcheng1230, we can accept multiple types for env_per_trial parameter.
What do you think about this name @tenzen-y @johnugeorge ?
There was a problem hiding this comment.
Here is the tentative implementation 12eea64 in which, the env_per_trial can be a list mixturing of V1EnvVar or V1EnvFromSource. Let me know your thoughts!
|
Can you rebase this PR since we fixed the optuna dependency issue? |
shipengcheng1230
force-pushed
the
add_env_settings
branch
from
12eea64
to
bab4e4d
Compare
|
Cool, done rebase and let me know what else is needed! |
| @@ -140,6 +141,7 @@ def tune( | |||
| parameters: Dict[str, Any], | |||
| base_image: str = constants.BASE_IMAGE_TENSORFLOW, | |||
| namespace: Optional[str] = None, | |||
| env_per_trial: Union[Dict[str, str], List[Union[V1EnvVar, V1EnvFromSource]], None] = None, | |||
There was a problem hiding this comment.
Should we make this type Optional and import libs from Kubernetes client, similar to other Kubernetes modules ?
| env_per_trial: Union[Dict[str, str], List[Union[V1EnvVar, V1EnvFromSource]], None] = None, | |
| env_per_trial: Optional[Union[Dict[str, str], List[Union[client.V1EnvVar, client.V1EnvFromSource]]]] = None, |
There was a problem hiding this comment.
Thanks for the great suggestion, I wasn't aware that those client.models are also importable from the client level!
|
Sorry for the late reply @shipengcheng1230, I left one small comment. |
shipengcheng1230
force-pushed
the
add_env_settings
branch
from
bab4e4d
to
3e552cf
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shipengcheng1230, tenzen-y The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Thank you for the update @shipengcheng1230! |
What this PR does / why we need it:
The
envandenv_fromcannot be directly set in the clienttunemethod. Recently I found myself need to set up proxy environment variables for cross-region data access. Doing so inside the existing python objective function seems less desirable than having them set through these two specifications.This PR enables this capability. By default, these two are set to None which aligns with V1Container. Therefore, behavior is not modified when not setting them.
Also, there was a missing of(Rebased the master change)cmaespackage when testing optuna suggestion. Therefore I added it in the optuna requirements.txt.