-
Notifications
You must be signed in to change notification settings - Fork 6k
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] Allow uv cache at installation #49176
Conversation
Signed-off-by: hjiang <dentinyhao@gmail.com>
43b39b3
to
6d44972
Compare
python, | ||
"-m", | ||
"uv", | ||
"pip", | ||
"install", | ||
"--no-cache", | ||
"" if self._uv_config.get("enable_uv_cache", False) else "--no-cache", |
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 there a valid use case where we don't want cache?
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.
Yes, cache lies in shared filesystem, managed by pip or uv.
The side-effect for cache is larger disk consumption.
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.
Another option that might be worth considering to add is the link mode, which can have a pretty significant impact on cache installation performance depending on filesystem characteristics. Although this can also be configured by an env var (UV_LINK_MODE), so I would understand not wanting to expand the number of arguments here.
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.
uv pip install --help
displays tons of options. Instead of explicitly add support for each of them, one way is that we can have a single uv_pip_install_options
argument which is a list of strings (default is ["--no-cache"]), this way user has the most flexibility to specify whatever options they want. One downside is that user is more likely to shoot themselves in the foot but as long as we can surface good error messages it should be fine. WDYT @sfriedowitz @pcmoritz?
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.
That would work great for my use case!
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.
Sounds good! I updated my implementation based on the suggestion.
Signed-off-by: hjiang <hjiang@anyscale.com>
bbcfd66
to
6f4a7d6
Compare
Signed-off-by: hjiang <dentinyhao@gmail.com>
Signed-off-by: hjiang <dentinyhao@gmail.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
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.
LG
Signed-off-by: hjiang <hjiang@anyscale.com>
Signed-off-by: hjiang <hjiang@anyscale.com>
Hi @jjyao , could you please help me merge the PR? Thank you! |
Waiting for approval from doc team. |
Signed-off-by: hjiang <dentinyhao@gmail.com>
Signed-off-by: hjiang <dentinyhao@gmail.com> Signed-off-by: ujjawal-khare <ujjawal.khare@dream11.com>
See issue: #49131
This PR provides an option to enable cache for
uv
.