-
Notifications
You must be signed in to change notification settings - Fork 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
Fix calculation of PipProvider._known_depths #10482
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.
Is it possible to add a test for this so we don't break it in future? A unit test would be sufficient IMO.
I would love that. But currently |
It’s probably easiest to add a few unit tests (not functional tests) directly against the provider. Call Also, please add a news fragment as documented here: https://pip.pypa.io/en/stable/development/contributing/#news-entries |
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.
This looks great! I just recommend you to follow the @uranusjr's suggestions, and that's it.
I've never written a unit test beyond I tried copying and modifying https://github.com/pypa/pip/blob/main/tests/unit/resolution_resolvelib/test_requirement.py#L87 but I couldn't figure out how to access to the PipProvider._known_depths or how to make the requirements more complex. |
Perhaps something like this: def test_provider_known_depths(factory):
provider = PipProvider(
factory=factory,
constraints={},
ignore_dependencies=False,
upgrade_strategy="to-satisfy-only",
user_requested={"user-requested": Mock()},
)
# Ask for the user-requested requirement "user-requested".
# I don't remember what exactly the requirement and candidate classes to use; adjust as needed.
# Maybe we should also check the return value of get_preference as well.
provider.get_preference(
identifier="user-requested",
resolution={},
candidates={"user-requested": iter([Candidate("user-requested", "1.0")])},
information={"user-requested": iter([(Requirement("user-requested"), None)])},
)
assert provider._known_depths == {"user-requested": 1}
# Now ask for the preference for "transitive", a dependency of "user-requested".
# Again, perhaps we should also check the return value is as expected.
provider.get_preference(
identifier="transitive",
resolution={"user-requested": Candidate("user-requested", "1.0")},
candidates={"user-requested": iter([Mock()]), "transitive": iter([Mock()])},
information={
"user-requested": iter([(Requirement("user-requested"), None)]),
"transitive": iter([(Requirement("transitive"), Candidate("user-requested", "1.0")]),
},
)
assert provider._known_depths == {"user-requested": 1, "transitive": 2} |
Updated with my first attempt at a test. I took your template and then reverse engineered the minimum I could from debugging a real example. I skipped over Apologies if it's the wrong approach like I said before I'm not familiar with real world tests, and this far exceeded by understanding of the mechanics of the pip code base. |
Generally looks good! I think the setup can be simplified a bit though; the provider interface is designed to only guarantee the arguments sasitfy certain “protocols” (e.g. iterator, mapping), so it’s not necessarily to pull out all the resolvelib internals for that ( Also you’ll need to run linters to fix some linting issues. It’s most recommended to set up pre-commit for this, but if that’s too slow for you, running linters manually before pushing would be effective as well. |
Fixed
I was able to replace |
I think both pip/src/pip/_internal/resolution/resolvelib/provider.py Lines 164 to 165 in b55ec00
So you should be able to do something like my_package_matches = provider.find_matches(
identifier="my-package",
requirements={"my-package": iter([SpecifierRequirement(my_package_install_requirement)])},
incompatibilities={"my-package": iter([])},
) |
Done. I also created a function called |
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.
This should be ready if tests pass.
A quick note on this change in case it gets merged for a pip release without my other changes, I don't expect it will improve performance. In fact my gut feeling is it could cause slightly more situations where heavy backtracking happens than situations where it fixes heavy backtracking. My testing on performance only indicated improvements with the other PRs listed in #10479 (comment) |
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
Let's get this merged first so your other PR doesn't need approval to run CI. |
Simple fix for #10415
Replaces #10438 which got in to a state I could not fix.