-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Download requirements in parallel #3981
Conversation
I tried to give this a quick look, but for some reason it appears you've made a bunch of random whitespace changes throughout the files which makes it harder to review. |
@dstufft I tried making the pep8 check happier. I'll revert that for now. |
d950af9
to
b6abba0
Compare
@dstufft Any chance to get this reviewed? |
Wow, good job! We had a huge speed up for one of our projects. Went from 85 to 52 seconds on average without any flags. So almost 40% faster for us there. When adding the flag In total 120 dependencies, with many of them being Flask and SQLAlchemy related. |
The problem is that the UI needs to be rethought in order to work with installing in parallel. |
@thedrow It's not slower with caching enabled, quite the opposite. With But yeah, the UI needs to be rethought for this feature. Perhaps we can just use a single progress bar for the download progress of all dependencies, with text on the right of it explaining what's happening (and this will shift back and forth with info like
|
I have been thinking about doing something like this for awhile, and not just for downloads, however I hadn't yet gotten around to it so thanks for contributing! One thing that might be useful here, I had looked at possibly replacing our current progress bar library with tqdm which supports nested progress bars so we could do something like:
You can see more examples of this on https://pypi.org/project/tqdm/. It's not a slam dunk though, one big issue would be we'd need a solution for tqdm/tqdm#228. |
@dstufft Do you mind if the UI will just be dots for the MVP? That way the feature would land earlier. |
@dstufft What other parts are you imagining parallelism for? Would the installation part be helped by it? @thedrow Looks pretty easy to implement nested progress bars with |
@jmagnusson I'd love for some help. I'm not really available to finish all of this including UI work. |
I know the feeling and right now it's just like that over here too. I'll keep this PR in mind for when my schedule clears up. Great work so far though! |
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
@dstufft I'm gonna close this for now as I don't have time to finish it. I hope that the basic approach will aid others. |
This is an attempt to resolve #825.
It uses multiple threads to prepare the requirements.
I have vendored the concurrent.futures backport to 2.7 for that purpose.
There are multiple issues here that needs to be resolved before this PR can be merged:
InterruptibleMixin
did before. That functionality is currently commented out and needs to be somehow restored before we merge this.Is it faster? I don't know. I tried installing Django CMS with it and this PR takes ~10 seconds less to install. That's just a quick test. We probably need to test this better somehow.
The results
pip 8.1.2:
This PR: