Skip to content
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] Parse local pip/conda requirements files locally upon task/actor definition #18988

Merged
merged 8 commits into from
Sep 30, 2021

Conversation

architkulkarni
Copy link
Contributor

@architkulkarni architkulkarni commented Sep 29, 2021

Why are these changes needed?

When using Ray Client, the .remote() function for actors and tasks are run on the local driver and run a second time on the remote Client server.

Previously, runtime env dict validation took place in .remote(). This caused a bug where a local file path was specified inthe "pip" or "conda" field, because when the validation was run in the Client server, the local file wouldn't be present.

This PR fixes this by separating out the pip and conda validation and moving it to the __init__ and .options() methods for actors and tasks. Here when a filename string is given, it is parsed into the internal representation List[str] (for pip) or Dict (for conda). Even though .options() is called a second time on the client server, the validation is a no-op there because the conda and pip fields are already in the internal representation at that point.

Related issue number

Closes #18976

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 :(

@architkulkarni
Copy link
Contributor Author

architkulkarni commented Sep 30, 2021

(EDIT: should be fixed by the next commit) @edoakes A couple issues here.

  1. It looks like the client_convert function reruns the entire _remote() function, including the runtime env rewriting part that we just added above the client_convert call. In particular, that rewriting part gets called twice, once locally and once in the client server. Hence we see the following error:
ValueError: pip-install-test
requests==2.24.0
 is not a valid file

because it already got rewritten from filename -> file contents once, and during the second call it tries to interpret the file contents as a filename.

  1. Since our changes are still happening in _remote(), it's not clear how to locally test it, since the validation hasn't been moved to the init() or .options() part.

@architkulkarni architkulkarni changed the title [WIP] Parse local pip/conda requirements files before client_convert is called [WIP] Parse local pip/conda requirements files locally upon task/actor definition Sep 30, 2021
Copy link
Contributor

@edoakes edoakes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, hopefully this works :)

python/ray/actor.py Show resolved Hide resolved
@architkulkarni architkulkarni changed the title [WIP] Parse local pip/conda requirements files locally upon task/actor definition [runtime env] Parse local pip/conda requirements files locally upon task/actor definition Sep 30, 2021
@architkulkarni
Copy link
Contributor Author

This PR unfortunately does not address #19002, which I think probably has an unrelated root cause.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[runtime env] [Bug] Specifying pip "requirements.txt" per-task doesn't work on remote cluster
2 participants