[runtime env][core] Use Proto message RuntimeEnvInfo between user code and core_worker
#22856
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Catch-Bull
changed the title
RuntimeEnvInfo between user code and core_workerRuntimeEnvInfo between user code and core_worker
Catch-Bull
requested review from
jovany-wang,
kfstorm,
raulchen,
ericl and
fishbone
as code owners
architkulkarni
approved these changes
Mar 9, 2022
There was a problem hiding this comment.
Looks like a nice refactor! There's one possibly relevant test failure, let's make sure it's addressed before merging. https://buildkite.com/ray-project/ray-builders-pr/builds/26071#d9e6abc6-e1cb-46df-9cb9-ff606004c760/6-6512
python/ray/actor.py
Outdated
| @@ -397,6 +398,23 @@ def __call__(self, *args, **kwargs): | |||
| f"use '{self.__ray_metadata__.class_name}.remote()'." | |||
| ) | |||
|
|
|||
| def _parse_runtime_env(self, runtime_env: Optional[Union[dict, RuntimeEnv]]): | |||
There was a problem hiding this comment.
Thanks for the refactor :)
There was a problem hiding this comment.
A some function exists in remote_function.py. Can we move it to utils.py for reuse?
python/ray/runtime_env.py
Outdated
| which not contained in `ProtoRuntimeEnv` but in `ProtoRuntimeEnvInfo`, | ||
| such as `eager_install`. This function will extract those fields from | ||
| `RuntimeEnv` and create a new `ProtoRuntimeEnvInfo`, and serialize it. | ||
| """ |
There was a problem hiding this comment.
Thanks for adding the docstring!
jovany-wang
reviewed
Mar 9, 2022
java/api/src/main/java/io/ray/api/runtimeenv/RuntimeEnvInfo.java
Outdated
Show resolved
Hide resolved
SongGuyang
reviewed
Mar 9, 2022
python/ray/actor.py
Outdated
| @@ -397,6 +398,23 @@ def __call__(self, *args, **kwargs): | |||
| f"use '{self.__ray_metadata__.class_name}.remote()'." | |||
| ) | |||
|
|
|||
| def _parse_runtime_env(self, runtime_env: Optional[Union[dict, RuntimeEnv]]): | |||
There was a problem hiding this comment.
A some function exists in remote_function.py. Can we move it to utils.py for reuse?
java/api/src/main/java/io/ray/api/runtimeenv/RuntimeEnvInfo.java
Outdated
Show resolved
Hide resolved
Catch-Bull
force-pushed
the
change_runtime_env_protocol_between_processes
branch
from
e469b3c
to
994fc93
Compare
Catch-Bull
force-pushed
the
change_runtime_env_protocol_between_processes
branch
from
1fed33a
to
fb41a04
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
After this PR, the transfer of runtime_env information between user code and core_work is changed from RuntimeEnv to RuntimeEnvInfo, then it will be more convenient for us to add some fields that are inconvenient to put in RuntimeEnv, such as setup_runtime_env_seconds
Related issue number
Checks
scripts/format.shto lint the changes in this PR.