-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[Core] Merge Driver/Job's runtime environment when it conflicts #39208
[Core] Merge Driver/Job's runtime environment when it conflicts #39208
Conversation
# Driver's specified `runtime_env` | ||
{"env_vars": {"C": "c"}} | ||
|
||
# Driver's actual `runtime_env` (merged with Job's) |
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 assume by Driver's runtime env, you mean runtime_env specified in ray.init(), then the driver's actual runtime env can only be job's runtime env since ray.init runtime env only have effects on tasks and actors.
I'd avoid Driver's runtime_env, by saying runtime_env specified via ray.init()
Also there is another confusing thing: we normally say ray.init() runtime_env as job level runtime env but now there is another job level runtime env specified via job submission.
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 guess I will call it job-level runtime env specified ray job submit vs ray.init then
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 rewrote this part. Can you check?
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.
Some nits for style/consistency and clarification.
doc/source/cluster/running-applications/job-submission/quickstart.rst
Outdated
Show resolved
Hide resolved
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 good! Some minor concerns:
- It looks like we've duplicated the merging code for job->driver and the existing merging code for driver->actor/task. Ideally both of these cases would just call into a single helper function (maybe with an arg raise_exception: bool)
- Similarly the merge behavior is described in the docs in two different places, ideally there would be a single source of truth that we just link to from both places.
Runtime Environment Specified by Both Job and Driver | ||
"""""""""""""""""""""""""""""""""""""""""""""""""""" | ||
|
||
You can specify Ray driver's runtime environment via `ray.init(runtime_env=...)` or `ray job submit --runtime-env`. |
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.
Ray driver's runtime env can only be specified via job submission. ray.init runtime env has no effect on the driver.
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.
Oh great catch. ray.init()
runtime env only sets the default for any subsequent task or actor's runtime env
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.
wait, so if we do job submission, the driver script will already have a runtime env created? How does it work now? Is the runtime env created by supervisor actor first and the driver is started with the same config?
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.
It looks like it is already written in a pretty confusing way; https://docs.ray.io/en/latest/ray-core/handling-dependencies.html#specifying-a-runtime-environment-per-job (it considers as job submit & ray.init as "runtime env per job"). I think we should technically distinguish them in this case. Let me try rewriting this part
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.
@jjyao @architkulkarni I updated the doc based on the feedback again; 401bf31. Can you guys check again? if this doesn't satisfy the merge bar, I will create a meeting to discuss this.
Also @architkulkarni
It looks like we've duplicated the merging code for job->driver and the existing merging code for driver->actor/task. Ideally both of these cases would just call into a single helper function (maybe with an arg raise_exception: bool)
I think we shouldn't use the Python helper for everything because we'd like to keep the inheritance logic in cpp if possible (so that it will be language independent). That said, we ideally should merge the runtime env in cpp. It seems like the job submit runtime env inheritance is not even supported from other languages, so I think we can do it when we actually support them in multi languages.
Similarly the merge behavior is described in the docs in two different places, ideally there would be a single source of truth that we just link to from both places.
Addressed in 401bf31
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.
Is the runtime env created by supervisor actor first and the driver is started with the same config?
That's right, the driver is started by the actor as a subprocess so it inherits the cwd and env vars automatically without Ray having to do anything.
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.
Update looks good to me, just some nits about the text.
|
||
This ensures the runtime environment is installed on the cluster before the entrypoint script is run. | ||
If ``runtime_env`` is specified from ``ray.init(runtime_env=...)``, the runtime env is only applied to all children Tasks and Actors, not the entrypoint script (Driver) itself. |
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.
From what I've seen, tasks, actors and drivers are not capitalized elsewhere in the doc. It would be great if you could double check and make it consistent
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.
This was recommended from @angelinalg. I will just stick to this it for now.
…project#39208) When the job and driver both specifies the runtime env, driver's runtime env is ignored. This is a confusing behavior and we'd like to merge instead. This PR merges the runtime envs, and if there's a conflict raises an exception. User can override the job's runtime env by driver's runtime env by specifying the env var.
…project#39208) When the job and driver both specifies the runtime env, driver's runtime env is ignored. This is a confusing behavior and we'd like to merge instead. This PR merges the runtime envs, and if there's a conflict raises an exception. User can override the job's runtime env by driver's runtime env by specifying the env var.
…project#39208) When the job and driver both specifies the runtime env, driver's runtime env is ignored. This is a confusing behavior and we'd like to merge instead. This PR merges the runtime envs, and if there's a conflict raises an exception. User can override the job's runtime env by driver's runtime env by specifying the env var. Signed-off-by: Jim Thompson <jimthompson5802@gmail.com>
…project#39208) When the job and driver both specifies the runtime env, driver's runtime env is ignored. This is a confusing behavior and we'd like to merge instead. This PR merges the runtime envs, and if there's a conflict raises an exception. User can override the job's runtime env by driver's runtime env by specifying the env var. Signed-off-by: Victor <vctr.y.m@example.com>
Why are these changes needed?
When the job and driver both specifies the runtime env, driver's runtime env is ignored. This is a confusing behavior and we'd like to merge instead.
This PR merges the runtime envs, and if there's a conflict raises an exception. User can override the job's runtime env by driver's runtime env by specifying the env var.
Related issue number
Closes #37930
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.