-
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
Resolution speed improvement #3839
Conversation
sharply decreases slowness :)
sharply decreases slowness :)
LGTM. Might require a peek from @sdispater though. |
@@ -417,6 +420,12 @@ def __init__( | |||
def reachable(self) -> List["PackageNode"]: | |||
children: List[PackageNode] = [] | |||
|
|||
# skip already traversed packages | |||
if self.package in self.seen: |
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.
Just an idea: could/should this check be moved to https://github.com/python-poetry/poetry/pull/3839/files#diff-c2fd13b8785fbb20817a8c0e4ce7425249eaa6be7a4faeb1fad846cadccfbfadR363 (part of the depth-first-search method implementation)?
Something along the lines of:
...
for neighbor in node.reachable():
back_edges[neighbor.id].append(node)
if neighbor.id in visited:
continue
...
(it feels like the check is an optimization of the DFS algorithm, which is why I wondered if it could live closer to the dfs_visit
method rather than being attached to the nodes that the search runs over. I haven't performance evaluated this though, nor checked whether it would behave the same way as your change)
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.
@jayaddison quite possibly! I kept the fix as close to the original as possible from August 2020 (before the DFS was more clearly formalized in the code). I expect that the regular authors/maintainers who are more familiar with the internal structure and flow will know where to best plug this fix in. 🙂
@jguice I just tried your version and the latest master, and the latest works faster for me somehow. Maybe some |
Guys, dozens of dev teams are dying while waiting for resolution of this issue. Step it up pls. |
@meandmymind be sure to clear your cache between runs for the test to be more valid. I've found this to be both a combination of Also this fix is based off |
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.
Looks good to me 👍 Thanks a lot!
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Pull Request Check List
Resolves (at least some of): #2094
AddedUpdated tests for changed code.This PR contains a fix for slow dependency resolution that's been in use successfully since August 2020. I've updated it for the latest
master
code as of this writing. All tests are passing and only required some ops re-ordering due to the nature of the fix.The root problem was that the final (interim) dependency tree had many redundant nodes (especially in projects with deep dependency hierarchies). This fix skips "seen" nodes which drastically speeds up many scenarios.
I encourage everyone with resolution speed issues to try this version from my fork (until hopefully this PR gets accepted). Please comment on this PR if you find any new issues using this version, and also if you experience speed improvements. 🚀