-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
[Runtime Env] Make every ${ENV_VAR} replaceable #36187
[Runtime Env] Make every ${ENV_VAR} replaceable #36187
Conversation
Signed-off-by: SangBin Cho <rkooo567@gmail.com>
python/ray/_private/utils.py
Outdated
os.environ[key] = value.replace("${" + key + "}", os.environ.get(key, "")) | ||
else: | ||
os.environ[key] = value | ||
os.environ[key] = os.path.expandvars(value) |
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.
Looks like the Python implementation leaves the env var unchanged if it doesn't exist. E.g.,
unset ENV_VAR
# previous
${ENV_VAR} -> ""
# now
${ENV_VAR} -> ${ENV_VAR}
I feel like this should be fine (especially given this python implementation will work cross platform correctly). But if we have to keep this behavior, I can somehow write the custom implementation
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.
nice, using python's expandvars is fine
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.
Can you also update the doc (https://docs.ray.io/en/latest/ray-core/handling-dependencies.html) as well?
Currently it has
env_vars (Dict[str, str]): Environment variables to set. Environment variables already set on the cluster will still be visible to the Ray workers; so there is no need to include os.environ or similar in the env_vars field. By default, these environment variables override the same name environment variables on the cluster. You can also reference existing environment variables using ${ENV_VAR} to achieve the appending behavior. Only PATH, LD_LIBRARY_PATH, DYLD_LIBRARY_PATH, and LD_PRELOAD are supported. See below for an example:
Example: {"OMP_NUM_THREADS": "32", "TF_WARNINGS": "none"}
Example: {"LD_LIBRARY_PATH": "${LD_LIBRARY_PATH}:/home/admin/my_lib"}
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.
|
cc @jjyao it is ready for review. |
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.
Do we have tests already?
@jjyao yes! |
Merged it because the previous run succeeds, and I only made doc changes. |
In the master, we only allow a few env vars specified via ${} translatable. This is unnecessarily too restrictive. This PR allows every env vars to be translated if they are specified via ${ENV_VAR} Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
In the master, we only allow a few env vars specified via ${} translatable. This is unnecessarily too restrictive. This PR allows every env vars to be translated if they are specified via ${ENV_VAR} Signed-off-by: Victor <vctr.y.m@example.com>
Why are these changes needed?
In the master, we only allow a few env vars specified via ${} translatable. This is unnecessarily too restrictive. This PR allows every env vars to be translated if they are specified via ${ENV_VAR}
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.