-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Rework resolution ordering to consider "depth" #10032
Rework resolution ordering to consider "depth" #10032
Conversation
f9844f2
to
eb548fc
Compare
This comment has been minimized.
This comment has been minimized.
eb548fc
to
d0f7a23
Compare
d0f7a23
to
dae7ad7
Compare
It seems to me that an easy win here is to have a boolean for "is this user requested", for making sure we get to user requested requirements first. Changing the strategy between BFS vs DFS is... well, I'm torn. It's not going to solve the underlying problem but only shift the cases where the problem occurs. On the other hand, we don't have evidence either way for which set of "situation this happens in" is less common, and implementing + trying this out is the only way to know. I think what we really need is some way to determine "what candidates are implicated in this conflict" and then trigger some sort of back jumping/tree pruning in the resolver logic. That's something that'll need to happen in resolvelib side though, and annoyingly difficult. :) All this isn't to say "don't do this". Rather, that this is annoyingly difficult and using |
Maybe we already do this? |
Yes, we already do this.
The current strategy is not DFS. It resolves user-requested requirements first (i.e. BFS only for the first layer), but treats everything else the same. This brings some interesting attributes. Take the following setup as an example:
where setup.py has with open("requirements.txt") as f:
install_requires = f.read().split()
setup(name="my-project", version="1", install_requires=install_requires) Currently This PR is basically introducing a generalisation to the user-requested dependency optimisation. Rather than treating the user-requested (top-level) dependencies specially, all dependencies are assigned a depth (distance to the top-level dependencies), and are resolved in that order. User-requested dependencies are the shallowest (depth 1) and therefore resolved first (like they are right now), and dependencies of them have depth 2 and resolved next, and so on. |
dae7ad7
to
9cf2e60
Compare
9cf2e60
to
f5f9135
Compare
End goal is to resolve #9455.
This modifies
Provider.get_preference()
to try to calculate a package’s “depth” in the dependency graph, and resolve packages closer to the root first. This works pretty well for #9455.I also tweaked the preference logic a little to prioritise “shallow” dependencies over those with non-pinning operators. This means that previously
foo>2
would be preferred overbar
even if the latter is user-specified, but nowbar
is resolved first. This is from a few examples I tried that had the same priority logic issue (the one in #9455,apache-airflow[all]
, and #9187 (comment)); in a reasonably maintained and mature project, almost all dependencies would have some kind of version constraints, but those generally are so wide they are really not meaningfully better than bare requirements, so I de-prioritised that part.I’m not quite sure how we can know if this is indeed a better implementation in the big picture, however. We probably should roll this out with a feature flag (i.e. keep the current implement behind
--use-deprecated
), but even with that we can’t be sure about tradeoffs if some people report negative impact, because those being impacted positively won’t report anything 😟