Skip to content

[runtime env] remove the API ray.runtime_env.get_current_runtime_env#22620

Closed
SongGuyang wants to merge 2 commits intoray-project:masterfrom
antgroup:dev_runtime_env_api
Closed

[runtime env] remove the API ray.runtime_env.get_current_runtime_env#22620
SongGuyang wants to merge 2 commits intoray-project:masterfrom
antgroup:dev_runtime_env_api

Conversation

@SongGuyang
Copy link
Copy Markdown
Contributor

Why are these changes needed?

Remove the API ray.runtime_env.get_current_runtime_env() and use ray.get_runtime_context().runtime_env instead.

Related issue number

#22594

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@SongGuyang SongGuyang requested a review from edoakes February 24, 2022 11:15
@SongGuyang
Copy link
Copy Markdown
Contributor Author

@edoakes There is a new issue I want to discuss:
Currently, ray.get_runtime_context().runtime_env return a new dict instance so we can modify it directly for inheritance. Like:

Actor.options(runtime_env=ray.get_runtime_context().runtime_env.update(
            {"env_vars": {"A": "a", "B": "b"}}))

This use case is a little weird because runtime_env is a property API which often be seen as a reference. So, do we need to add a new API in runtime_context? Like:

Actor.options(runtime_env=ray.get_runtime_context().get_runtime_env().update(
            {"env_vars": {"A": "a", "B": "b"}}))

@edoakes
Copy link
Copy Markdown
Collaborator

edoakes commented Feb 24, 2022

@SongGuyang it's not safe to return a reference to the worker's runtime_env anyways (don't want users messing with it), so I think let's just document that it returns a copy

@edoakes edoakes added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Feb 24, 2022
@edoakes
Copy link
Copy Markdown
Collaborator

edoakes commented Feb 24, 2022

linter failing on docs

@edoakes
Copy link
Copy Markdown
Collaborator

edoakes commented Mar 1, 2022

@SongGuyang ping to fix tests and get this merged

@SongGuyang
Copy link
Copy Markdown
Contributor Author

@edoakes do we still need this PR? The previous PR has been reverted.

@edoakes
Copy link
Copy Markdown
Collaborator

edoakes commented Mar 2, 2022

Yes we should still merge this; it updates tests and removes the currently unused code

@edoakes
Copy link
Copy Markdown
Collaborator

edoakes commented Mar 2, 2022

Oh, sorry, now I realize that this makes no sense after the revert.... I was just looking at the diff but didn't realize it doesn't apply to master. My bad.

@edoakes edoakes closed this Mar 2, 2022
@SongGuyang
Copy link
Copy Markdown
Contributor Author

OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants