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

[core] [2/N] Implement uv processor #48486

Merged
merged 27 commits into from
Nov 6, 2024

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Nov 1, 2024

This PR does two things:

  • Implement the uv and package installation
  • Extract common utils between uv and pip into multiple util files

Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
python/ray/_private/runtime_env/uv_test.py Outdated Show resolved Hide resolved
python/ray/_private/runtime_env/uv_test.py Outdated Show resolved Hide resolved
@dentiny dentiny mentioned this pull request Nov 1, 2024
3 tasks
@dentiny dentiny requested a review from jjyao November 1, 2024 20:25
@@ -29,28 +29,28 @@ steps:

# tests
- label: ":database: data: arrow v9 tests"
tags:
tags:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

linter makes all these changes, otherwise I cannot push

Copy link
Collaborator

Choose a reason for hiding this comment

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

could you split the unrelated lint changes to another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good suggestion and I thought about it also, my confusion is, when I tried to push on the master branch, prepush hook doesn't seem to work :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

you can just checkout the changes of these files.

I think the hook is configured to fix changed files only.

the people who added the pre-commit's should have fixed all the lint errors across the entire repo first.. and then run it on CI with -a flag..

@@ -49,13 +49,13 @@ in combination with the following scripts and command lines in order to run RLli
### [Atari100k](../../tuned_examples/dreamerv3/atari_100k.py)
```shell
$ cd ray/rllib/tuned_examples/dreamerv3/
$ python atari_100k.py --env ale_py:ALE/Pong-v5
$ python atari_100k.py --env ale_py:ALE/Pong-v5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prepush linter hook change, otherwise I cannot push

Copy link
Collaborator

Choose a reason for hiding this comment

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

what prepush linter are you using? the pre-commit one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I add it to my git pre-push hook, which automatically fixes linting issues before push.

Copy link
Contributor Author

@dentiny dentiny Nov 5, 2024

Choose a reason for hiding this comment

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

PR: #48557, I guess it needs to be merged before this PR, otherwise cannot update and push still. :(

@dentiny dentiny added the go add ONLY when ready to merge, run all tests label Nov 5, 2024
@dentiny dentiny requested a review from khluu November 5, 2024 01:43
Copy link
Collaborator

@jjyao jjyao left a comment

Choose a reason for hiding this comment

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

LG.

Also rebase with master to for [1/N]

python/ray/_private/runtime_env/env_utils.py Outdated Show resolved Hide resolved
python/ray/_private/runtime_env/path_utils.py Outdated Show resolved Hide resolved
python/ray/_private/runtime_env/path_utils.py Outdated Show resolved Hide resolved
python/ray/_private/runtime_env/uv.py Outdated Show resolved Hide resolved
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
@dentiny
Copy link
Contributor Author

dentiny commented Nov 5, 2024

Also rebase with master to for [1/N]

This PR has already rebased upon first PR, which gets merged earlier today :)

Signed-off-by: hjiang <hjiang@anyscale.com>
@dentiny dentiny requested a review from jjyao November 5, 2024 06:04
python/ray/tests/unit/test_uv.py Outdated Show resolved Hide resolved
python/ray/_private/runtime_env/uv.py Show resolved Hide resolved
python/ray/_private/runtime_env/uv.py Show resolved Hide resolved
python/ray/_private/runtime_env/uv.py Outdated Show resolved Hide resolved
@@ -310,6 +311,7 @@ def __init__(
_validate: bool = True,
mpi: Optional[Dict] = None,
image_uri: Optional[str] = None,
uv: Optional[str] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this type annotation right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just revisited, I think both pip and uv type annotation is incorrect, fixed.

runtime_env = TestRuntimeEnv()

uv_processor = uv.UvProcessor(target_dir=target_dir, runtime_env=runtime_env)
await uv_processor._run()
Copy link
Collaborator

Choose a reason for hiding this comment

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

What we are testing here?

Copy link
Contributor Author

@dentiny dentiny Nov 5, 2024

Choose a reason for hiding this comment

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

The whole uv process only does download work, which is not expected in unit test.

For here, I simply goes though the initialization and mocks all the IO operations out.
It might sound weird, but I did uncover a few bugs during UT (i.e. pip not correctly replaced with uv).

Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
@dentiny dentiny requested a review from jjyao November 5, 2024 06:56
python/ray/_private/runtime_env/uv.py Outdated Show resolved Hide resolved
python/ray/_private/runtime_env/uv.py Outdated Show resolved Hide resolved
Comment on lines 359 to 360
"The 'pip' field, 'conda' field and 'uv' field of "
"runtime_env cannot both be specified.\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only one of 'pip' field, 'conda' field and 'uv' field of runtime_env can be specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow with another comment, revert all exposed public interfaces.

Comment on lines 239 to 240
uv: Either a list of pip packages, or a python dictionary that has one fields:
packages (required, List[str]): a lists of uv packages.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add the doc when 'uv' is usable. With this PR, people cannot use 'uv' plugin yet.

I think changes to this file should be in the next PR when we actually implement the uv plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! Updated.

Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
@dentiny dentiny requested a review from jjyao November 5, 2024 18:27
python/ray/_private/runtime_env/uv.py Show resolved Hide resolved
python/ray/_private/runtime_env/uv.py Show resolved Hide resolved
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
@dentiny
Copy link
Contributor Author

dentiny commented Nov 6, 2024

@jjyao Finally I cleaned up all the stale usage after the refactor, could you please help me merge the PR? Thank you!

@jjyao jjyao merged commit a49978f into ray-project:master Nov 6, 2024
5 checks passed
JP-sDEV pushed a commit to JP-sDEV/ray that referenced this pull request Nov 14, 2024
Signed-off-by: hjiang <hjiang@anyscale.com>
mohitjain2504 pushed a commit to mohitjain2504/ray that referenced this pull request Nov 15, 2024
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: mohitjain2504 <mohit.jain@dream11.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants