Skip to content
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

chore(deps): update resolvelib to 1.1.0 #3235

Merged
merged 2 commits into from
Oct 31, 2024
Merged

Conversation

frostming
Copy link
Collaborator

Signed-off-by: Frost Ming me@frostming.com

Pull Request Checklist

  • A news fragment is added in news/ describing what is new.
  • Test cases added for changed code.

Describe what you have changed in this PR.

Signed-off-by: Frost Ming <me@frostming.com>
Signed-off-by: Frost Ming <me@frostming.com>
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.42%. Comparing base (4401ff5) to head (bb14c37).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3235      +/-   ##
==========================================
- Coverage   85.42%   85.42%   -0.01%     
==========================================
  Files         112      112              
  Lines       11323    11317       -6     
  Branches     2459     2457       -2     
==========================================
- Hits         9673     9667       -6     
  Misses       1125     1125              
  Partials      525      525              
Flag Coverage Δ
unittests 85.21% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@frostming frostming merged commit efaccb8 into main Oct 31, 2024
24 checks passed
@frostming frostming deleted the dep/update-resolvelib branch October 31, 2024 02:18
@notatallshaw
Copy link

Is there a reason you got rid of depth as a preference?

I ask this because I am taking a look at pip's implementation and I have found at least one major bug. My experience looking at how uv performs compared to pip leads me to think depth is a general net positive, so I am going to try and fix the bug. But do you have a reason to think it's worth dropping altogether?

@woopla woopla mentioned this pull request Oct 31, 2024
1 task
@frostming
Copy link
Collaborator Author

frostming commented Nov 1, 2024

Thanks for asking,

First but not least, the depth is no longer available on resolvelib 1.1, it breaks, and hence the easy(and lazy) fix.
Secondly, pip doesn't have depth in the preference, so I think it's okay to remove it.
Indeed, depth was introduced to prevent the resolver from going too deep in a subtree at an early stage, but rather use a BFS-like algorithm.
But now we have backjumping so I believe this problem can be mitigated.

And after all I just want to see wether it works well.

@notatallshaw
Copy link

First but not least, the depth is no longer available on resolvelib 1.1, it breaks, and hence the easy(and lazy) fix.

Really? I thought this was something the provider implemented, not something resolvelib specifically offered?

Secondly, pip doesn't have depth in the preference, so I think it's okay to remove it.

Is this not the depth preference in pip:
https://github.com/pypa/pip/blob/24.3.1/src/pip/_internal/resolution/resolvelib/provider.py#L166-L170
https://github.com/pypa/pip/blob/24.3.1/src/pip/_internal/resolution/resolvelib/provider.py#L193

I don't get an error from it when I vendor resolvelib 1.1.0: pypa/pip#13001 and the known_depths tests don't fail, but I'll investigate a bit further on the output.

I have found that it fails to calculate depth correctly when extras are involved, but that's not new.

@notatallshaw
Copy link

Also, FYI, I've been building a series of real world resolutions that are big and problamatic so I can test whether a particular change to the resolver causes problems or not:

The tooling itself is a little rough, but I'll improve it over time.

@frostming
Copy link
Collaborator Author

frostming commented Nov 1, 2024

Is this not the depth preference in pip:
https://github.com/pypa/pip/blob/24.3.1/src/pip/_internal/resolution/resolvelib/provider.py#L166-L170
https://github.com/pypa/pip/blob/24.3.1/src/pip/_internal/resolution/resolvelib/provider.py#L193

I overlooked

Really? I thought this was something the provider implemented, not something resolvelib specifically offered?

Yes, let me clarify. Previously, the provider relies on the calling of get_preference to populate the values of known_depth, but since this has been changed in resolvelib 1.1: https://github.com/sarugaku/resolvelib/blob/90574f376fbc2aec659c1c3f7c436937f2c398a3/src/resolvelib/resolvers/resolution.py#L438-L442

The get_preference method is not called in the second branch, causing missing keys in known_depth. I didn't read pip's code carefully but I assume it should also be a problem.

To reproduce you can try a single dependency with only one transitive dependency. A -> B -> [...]

@notatallshaw
Copy link

Yes, let me clarify. Previously, the provider relies on the calling of get_preference to populate the values of known_depth, but since this has been changed in resolvelib 1.1: https://github.com/sarugaku/resolvelib/blob/90574f376fbc2aec659c1c3f7c436937f2c398a3/src/resolvelib/resolvers/resolution.py#L438-L442

The get_preference method is not called in the second branch, causing missing keys in known_depth. I didn't read pip's code carefully but I assume it should also be a problem.

To reproduce you can try a single dependency with only one transitive dependency. A -> B -> [...]

Gotcha, this was my change, and not intentional.

I'm going to investigate a bit and test adding depth back, there was a significant performance regression in this resolution and I wonder if it was because of losing depth: https://github.com/notatallshaw/Pip-Resolution-Scenarios-and-Benchmarks/blob/main/scenarios/problematic.toml#L58

If I do find out it causes issues, I think the best way to restore a provider being able to construct depth is to always call narrow_requirement_selection rather than having a branch here: https://github.com/sarugaku/resolvelib/blob/90574f376fbc2aec659c1c3f7c436937f2c398a3/src/resolvelib/resolvers/resolution.py#L414. That way a provider can choose to construct depths in narrow_requirement_selection while still being able to narrow the selection.

@notatallshaw
Copy link

Okay, I spent the weekend running and adding scenarios, and the conclusions were pretty interesting:

  1. Fixing depth preference only makes performance worse in my scenarios
  2. Pip's current broken implementation performs slightly better than removing depth preference altogether 🙃
  3. Removing depth does cause pip to take significantly longer wall clock time to hit ResolutionTooDeep , so probably worth reducing max backtrack number

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants