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] [9/N] Skip uv install if match #48668

Closed

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Nov 9, 2024

Followup on #48634, which skip installation if specified version matches what we have in virtual env.

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>
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 dentiny requested review from jjyao and rynewang November 9, 2024 05:16
@dentiny dentiny added the go add ONLY when ready to merge, run all tests label Nov 9, 2024
@dentiny dentiny changed the title [core] Skip uv install if match [core] [9/N] Skip uv install if match Nov 9, 2024
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
@jjyao
Copy link
Collaborator

jjyao commented Nov 12, 2024

I think we can always reinstall to make the code simpler which should be an no-op by uv?

@dentiny
Copy link
Contributor Author

dentiny commented Nov 12, 2024

I think we can always reinstall to make the code simpler which should be an no-op by uv?

If user requested version is always installed on the env, why bother re-installing?

@dentiny
Copy link
Contributor Author

dentiny commented Nov 13, 2024

I think we can always reinstall to make the code simpler which should be an no-op by uv?

The win for this PR is to reduce runtime overhead by pip, which is known to be ineffective.
I would like to reduce runtime setup invocation via pip as much as possible.

Comment on lines +126 to +131
# If exists, the output format would look like
# uv <version> (<sha> <release date>), for example,
# uv 0.5.1 (f399a5271 2024-11-08)
version_strs = version_output.split()
if len(version_strs) == 4 and version_strs[0] == "uv":
return version_strs[1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is fragile. The output format can change at anytime. Do we have other way to get the version?

@jjyao
Copy link
Collaborator

jjyao commented Nov 13, 2024

The win for this PR is to reduce runtime overhead by pip, which is known to be ineffective.

I was thinking that pip should do nothing if the specified version is already installed (basically pip does the skip for us)

@dentiny
Copy link
Contributor Author

dentiny commented Nov 13, 2024

I was thinking that pip should do nothing if the specified version is already installed (basically pip does the skip for us)

That makes sense, though I'm thinking to skip pip operation directly.

@dentiny
Copy link
Contributor Author

dentiny commented Nov 13, 2024

@jjyao I redo the PR just to skip "enforce reinstallation": #48731

@dentiny dentiny closed this Nov 13, 2024
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.

2 participants