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

Add UV_CONCURRENT_INSTALLS variable in favor of RAYON_NUM_THREADS #3646

Merged
merged 2 commits into from
May 18, 2024

Conversation

ibraheemdev
Copy link
Member

Summary

Continuation of #3493. This gives us more flexibility in case we decide to move away from rayon in the future.

Should this be named something else? We also use rayon for unzipping files, and maybe other things in the future?

@ibraheemdev ibraheemdev added the configuration Settings and such label May 17, 2024
@zanieb
Copy link
Member

zanieb commented May 17, 2024

Are we always going to use a single global thread pool?

I guess... UV_WORKER_THREADS would be a more general name?

@charliermarsh
Copy link
Member

It seems like an implementation detail though. If, in the future, we use Rayon elsewhere, we may in fact want to use different pool sizes for different tasks -- or is that wrong?

@ibraheemdev
Copy link
Member Author

Yeah. For example we run some other file operations on tokios blocking pool. I think worker threads is a bit misleading because we already have download and build limits.

@charliermarsh
Copy link
Member

I think the current name is ok...

@@ -583,8 +585,6 @@ In addition, uv respects the following environment variables:
- `FISH_VERSION`: Used to detect the use of the Fish shell.
- `BASH_VERSION`: Used to detect the use of the Bash shell.
- `ZSH_VERSION`: Used to detect the use of the Zsh shell.
- `RAYON_NUM_THREADS`: Used to control the number of threads used when unzipping and installing
packages. See the [rayon documentation](https://docs.rs/rayon/latest/rayon/) for more.
Copy link
Member

Choose a reason for hiding this comment

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

Does this no longer have any effect? (If it still has an effect, we should leave it here, even if UV_CONCURRENT_INSTALLS is recommended, since this is intended to document env variables that could affect behavior.)

Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn't have any effect because we're initialize the global thread pool manually.

// The default concurrent builds limit.
pub fn default_builds() -> usize {
// The default concurrent builds and install limit.
pub fn threads() -> usize {
Copy link
Member

Choose a reason for hiding this comment

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

Just confirming that this is the same as the Rayon default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Feel free to close that issue once merged IMO.

@ibraheemdev ibraheemdev merged commit 5363339 into astral-sh:main May 18, 2024
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Settings and such
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants