pythonCatchConflictsHook: prevent exponential worst-case #305619
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ofborg
bot
added
10.rebuild-darwin-stdenv
phaer
approved these changes
Apr 20, 2024
wegank
added
the
12.approvals: 1
Add a test where a conflicting package can be found at the end of multiple dependency chains. This is far too simple an example to demonstrate the ill effects of exponential time complexity, but does serve to demonstrate how the error output changes when each path is only visited once.
The hook performs a depth first search on the graph defined by propagatedBuildInputs. This traverses all paths through the graph, except for any cycles. In the worst case with a highly connected graph, this search can take exponential time. In practice, this means that in cases with long dependency chains and multiple packages depending on the same package, the hook can take several hours to run. Avoid this problem by keeping track of already visited paths and only visiting each path once. This makes the search complete in linear time. The visible effect of this change is that, if a conflict is found, only one dependency chain that leads to the conflicting package is printed, rather than all the possible dependency chains.
Now that we only visit each path once, a few things can be simplified. We no longer have to keep a list of different dependency chains leading to a package, since only one chain will ever be found. Also, the already visited check also takes care of cycles, so the other cycle check can be removed.
…itespace Currently, nix-support/propagated-build-inputs is parsed by splitting on a single space. This means that if this file contains multiple spaces separating two paths, the build_inputs list will end up containing an empty string. Instead, call split() with no arguments, which splits on runs of whitespace and also ignores whitespace at the beginning and end of the string, eliminating the need for strip().
lopsided98
force-pushed
the
catch-conflicts-exponential
branch
from
8d18ed3
to
f9de72f
Compare
|
I updated the target branch |
wegank
removed
the
12.approvals: 1
phaer
approved these changes
Apr 27, 2024
phaer
added
the
12.approvals: 1
mweinelt
requested review from
Lassulus and
DavHau
and removed request for
jonringer
DavHau
approved these changes
Apr 28, 2024
There was a problem hiding this comment.
LGTM
Thanks for the fix. It looks good to be merged.
I think it would be better if it would display the first 5 paths leading to a collision instead of just one. I guess this would still be in linear time and it would allow to better grasp the complexity of the underlying dependency conflict and make it easier to debug it.
But I don't want to block this. This could be done in a separate PR
wegank
added
12.approvals: 2
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
The hook performs a depth first search on the graph defined by
propagatedBuildInputs. This traverses all paths through the graph, except for any cycles. In the worst case with a highly connected graph, this search can take exponential time. In practice, this means that in cases with long dependency chains and multiple packages depending on the same package, the hook can take several hours to run.Avoid this problem by keeping track of already visited paths and only visiting each path once. This makes the search complete in linear time.
The visible effect of this change is that, if a conflict is found, only one dependency chain that leads to the conflicting package is printed, rather than all the possible dependency chains.
Now that only one dependency chain will be found for each package, a few other simplifications are possible and are included in a separate commit.
Also, add a test to demonstrate the change in output with multiple dependency chains, and fix a minor bug in parsing
nix-support/propagated-build-inputs.The tests pass and I have tested building a few Python packages.
Fixes #305599
cc @DavHau @phaer
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.