-
Notifications
You must be signed in to change notification settings - Fork 2.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
New and faster installer implementation #2595
Conversation
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.
A few recommendations. Haven't gone through the test cases yet and have largely skimmed the UI part of the executor.
installer = Installer( | ||
self.io, self.env, self.poetry.package, self.poetry.locker, self.poetry.pool | ||
self._installer.use_executor( | ||
self.poetry.config.get("experimental.new-installer", False) |
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.
Should we consider moving this into a configuration constants section?
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.
Some, not 100% related to this PR, comments. Lemme know if this is less than useful. :)
hi. excited to try this out! when i tried it i get a i wonder if it's because we have |
@davidszotten Thanks for reporting this! There were actually missing elements for |
@abn Your various points have been addressed and/or fixed. |
for task in as_completed(tasks): | ||
task.result() | ||
except KeyboardInterrupt: | ||
self._shutdown = True |
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.
could we not just (cancel tasks and) shutdown the executor and just return here? if i'm reading correctly nothing else happens if shutdown
becomes True
anyway
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.
It's just to have the same code path whether we have a KeyboardInterrupt or an actual error, since an error will also set _shutdown
.
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.
ah, missed that the tasks can also set replied to the wrong comment_shutdown
. thanks
hm, something else has gone wrong in 35225a7. before, the status message (Pending, Downloading, Installing) would disappear after each package finished installing. Now it remains next to each package (so i can no longer tell what is still installing from what is done) |
@davidszotten Yes, I noticed that as well but it should be fixed now. |
@@ -304,6 +304,13 @@ def package(self, name, version, extras=None): # type: (...) -> Package | |||
|
|||
return package | |||
|
|||
def find_links_for_package(self, package): |
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.
minor, but you missed the type annotation here :) have you considered turning on disallow_untyped_defs
?
Thanks to everyone who reviewed this and especially @davidszotten for testing it thoroughly. I am pretty confident that this is in a stable enough state to be released in the next alpha release of the If further bugs and issues are discovered we will address them in the next alpha (which should be the last) and beta releases. |
🎉 |
@sdispater is it possible to track what makes an installation slower? I'd like to debug the below install time to see if any package is causing problems.
|
@danihodovic I am unable to reproduce that in a new project. $ poetry@1.1.0rc1 add watchdog
Using version ^0.10.3 for watchdog
Updating dependencies
Resolving dependencies... (0.1s)
Writing lock file
Package operations: 2 installs, 0 updates, 0 removals
• Installing pathtools (0.1.2)
• Installing watchdog (0.10.3) Just to make sure we are not missing anything, what version of poetry are you using? If you are using |
I'm not interested in opening a new bug on the slow dependency resolution as there are plenty open already. I want to know how to debug it when the dependency resolution takes an unreasonable amount of time. |
Use |
I've got some problems in gitlab ci using new poetry parallel installation.
I don't even know which package causes this. It is likely not related to comet_ml as it is different package everytime. but it can be fixed with |
We also face occasional errors when using new version of Poetry:
For now we decided to run
|
@earshinov you can disable the new installer using |
Locking this thread as issues reported here might be missed and get lost. If there are issues with the installer, please first search existing issues. If an issue does not exist, please create one with details on how to reproduce it, For issues related to the new installer, if a fix has not already been made for your issue, you may want to disable the new installer like so until the issue has been resolved. poetry config experimental.new-installer false |
Pull Request Check List
This PR introduces a new installation experience with a brand new installer that supports parallel operations. This was made possible by splitting the different parts of the installation process into several new classes:
Executor
class is now the main point of entry and will be used by the existingInstaller
class if opted in via theuse_executor
method (note that the new installer will be opt-out by default since I want to gather as much feedback as possible). TheExecutor
is responsible for executing a batch of operations (install, uninstall , update), in parallel by default. Note that not all operations will be executed in parallel but only those with the same depth in the dependency tree.Chooser
class is responsible for picking the appropriate distributions (sdist or wheel) for a given package and environment.Chef
class does not do much at the moment, it only manages the distributions cache, but will eventually be responsible for building wheels before package installation, this will be implemented in the1.2
release.Authenticator
class is reponsible for handling credentials when downloading distributions from custom indices. Most of the ideas have been taken from the cache system ofpip
.This is the first step towards the removal of calls to
pip
for installation of packages. We now use it exclusively to install distributions available locally.The installer can be opted out via the
experimental.new-installer
setting:Here is an example of the new installer output: