Fix calculation of PipProvider._known_depths

[notatallshaw]

Simple fix for #10415

Replaces #10438 which got in to a state I could not fix.

[pfmoore]

Is it possible to add a test for this so we don't break it in future? A unit test would be sufficient IMO.

[notatallshaw]

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 get_preference has no unit tests, if someone could give me some hints on on how to start I'll give it a go.

[uranusjr]

It’s probably easiest to add a few unit tests (not functional tests) directly against the provider. Call provider.get_preferences a few times and observe whether provider._known_depths changes as expected.

Also, please add a news fragment as documented here: https://pip.pypa.io/en/stable/development/contributing/#news-entries

[DiddiLeija]

This looks great! I just recommend you to follow the @uranusjr's suggestions, and that's it.

[notatallshaw]

It’s probably easiest to add a few unit tests (not functional tests) directly against the provider. Call provider.get_preferences a few times and observe whether provider._known_depths changes as expected.

I've never written a unit test beyond assert 1 + 1 = 2 learn your first test case style tests, I need an example to get started on.

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.

[uranusjr]

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}

[notatallshaw]

Perhaps something like this:

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 resolution and candidates because they are not actually used by the get_preference method.

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.

[uranusjr]

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 (IteratorView et al.) since those only exist as an optimisation (so the resolver does not need to repeatedly build new dicts and lists which is wasteful); in tests, however, you can simply build dicts and iterators directly.

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.

[notatallshaw]

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

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 (IteratorView et al.) since those only exist as an optimisation (so the resolver does not need to repeatedly build new dicts and lists which is wasteful); in tests, however, you can simply build dicts and iterators directly.

I was able to replace build_iter_view with iter, nothing else I tried replacing worked.

[uranusjr]

I think both requirements and incompatibilities can be simplified. Pip’s provider implementation only expects primitive types (remembering dict is a mapping):

pip/src/pip/_internal/resolution/resolvelib/provider.py

Lines 164 to 165 in b55ec00

	requirements: Mapping[str, Iterator[Requirement]],
	incompatibilities: Mapping[str, Iterator[Candidate]],

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([])},
)

[notatallshaw]

I think both requirements and incompatibilities can be simplified. Pip’s provider implementation only expects primitive types (remembering dict is a mapping):

Done.

I also created a function called build_package_criterion, which I think makes it easier to see what's going on in the test by removing a large chunk of the boilerplate. It might also be useful later for testing issues with other PipProvider properties.

[uranusjr]

This should be ready if tests pass.

[notatallshaw]

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)

[uranusjr]

Let's get this merged first so your other PR doesn't need approval to run CI.