Make BacktrackingResolver ignore extras when dropping existing constraints

[chludwig-haufe]

This PR resolves #1977.

When the backtracking resolver collected the names of incompatible install requirements, it used to apply utils.key_from_req to the requirements listed in the exception causes. According to the type hints, utils.key_from_req expects either a pip._internal.req.req_install.InstallRequirement or a pip._vendor.packaging.requirements.Requirement. In some cases, though, the object passed is a pip._internal.resolution.resolvelib.requirements.SpecifierRequirement (i.e., a specialization of pip._internal.resolution.resolvelib.base.Requirement).

All mentioned classes define an attribute name. In case of InstallRequirement and the Requirement from the vendor package, the name holds the bare package name. In case of the Requirement from the resolvelib package, the name includes the extras (if any).

key_from_req is used in other places as well, I don't know whether they rely on the existing behavior. Therefore, I added a new function utils.key_no_extra_from_req that also accepts instances of pip._internal.resolution.resolvelib.base.Requirement and keeps only the part of the name up to the first opening square bracket (if any).

I was surprised I did not find more tests of the backtracking resolver. In the end, I followed the example of test_catch_distribution_not_found_error and directly called the backtracking resolver's method _do_resolve with a mocked pip resolver object.

Furthermore, I had to add pytest to the config of the mypy-mirror pre-commit hook.

I still got "untyped def" errors from test_resolver.py, even though there's an override for test modules in pyproject.toml that is supposed to disable this check. I therefore had the impression that mypy-mirror ignores the mypy config in pyproject.toml. I didn't investigate this any further, though, and simply annotated the all parameters of my test.

Contributor checklist

• Included tests for the changes.
• PR title is short, clear, and ready to be included in the user-facing changelog.

Maintainer checklist

• Verified one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
• Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

[chludwig-haufe]

I found that piptools.utils already contained a function strip_extras() (line 478). Therefore, I switched to using the existing function. I don't see any reason to duplicate this functionality.

[chrysle]

I wasn't aware of that function. Say, also since it's already used in resolver.py, wouldn't it be easier to just use it on the output of key_from_req? What do you think?

Sorry for pushing you around.

[chludwig-haufe]

No problem, sometimes implementations need multiple iterations before the best approach becomes apparent.

I will take a look in the evening how to simplify the PR this way.

[atugushev]

Thanks for the fix! While I don't mind unit tests, I'd prefer having an integration test instead. Mocking around with private methods like Resolver._do_resolve() could lead to extra work during future internal refactoring.

Could you please add at least one "success" integration test in test_cli_compile.py to confirm that the issue is resolved? Use test_cli_compile_strip_extras as a reference.

[chludwig-haufe]

@atugushev: I replaced the unit test with an integration test as you proposed.

I gather from pyproject.toml (line 95ff) that your intention is to ignore untyped definitions in the tests. For some reason, mypy called from the pre-commit hook insists on typed definitions on all functions I touched in the tests, including the functions passed in as fixtures.

These functions have "complex" signatures (too complex for Callable, at least), whence I had to define protocol classes for them. Furthermore, I cannot reference these protocols if I simply define them next to the fixtures in conftest.py. Therefore, I placed them into tests/utils.py.

[chrysle]

I gather from pyproject.toml (line 95ff) that your intention is to ignore untyped definitions in the tests. For some reason, mypy called from the pre-commit hook insists on typed definitions on all functions I touched in the tests, including the functions passed in as fixtures.

This however sounds like an issue that should be investigated. Haven't found something loadable yet, though.

[chludwig-haufe]

@chrysle: For demonstration's sake, I added --verbose to the mypy arguments in .pre-commit-config.yaml and then removed the type hint for the parameter make_package of test_resolver_drops_existing_conflicting_constraint() in test_cli_compile.py.

pre-commit rejected this commit, here's the relevant output:

mypy.....................................................................Failed
- hook id: mypy
- exit code: 1
LOG:  Mypy Version:           1.5.1
LOG:  Config File:            pyproject.toml
LOG:  Configured Executable:  /Users/christoph.ludwig/.cache/pre-commit/repo3zt33lob/py_env-python3.11/bin/python
LOG:  Current Executable:     /Users/christoph.ludwig/.cache/pre-commit/repo3zt33lob/py_env-python3.11/bin/python
LOG:  Cache Dir:              .mypy_cache
LOG:  Compiled:               True
LOG:  Exclude:                ['^tests/test_data/']
LOG:  Found source:           BuildSource(path='tests/test_cli_compile.py', module='tests.test_cli_compile', has_text=False, base_dir='/Users/christoph.ludwig/Python/pip-tools', followed=False)
LOG:  Could not load cache for tests.test_cli_compile: tests/test_cli_compile.meta.json
LOG:  Metadata not found for tests.test_cli_compile
LOG:  Parsing tests/test_cli_compile.py (tests.test_cli_compile)
LOG:  Metadata fresh for tests: file /Users/christoph.ludwig/Python/pip-tools/tests/__init__.py
LOG:  Metadata fresh for piptools.scripts.compile: file /Users/christoph.ludwig/Python/pip-tools/piptools/scripts/compile.py
LOG:  Metadata fresh for unittest.mock: file /Users/christoph.ludwig/.cache/pre-commit/repo3zt33lob/py_env-python3.11/lib/python3.11/site-packages/mypy/typeshed/stdlib/unittest/mock.pyi
LOG:  Metadata fresh for click.testing: file /Users/christoph.ludwig/.cache/pre-commit/repo3zt33lob/py_env-python3.11/lib/python3.11/site-packages/click/testing.py
LOG:  Metadata fresh for piptools.utils: file /Users/christoph.ludwig/Python/pip-tools/piptools/utils.py
LOG:  Metadata fresh for tests.constants: file /Users/christoph.ludwig/Python/pip-tools/tests/constants.py
LOG:  Metadata fresh for tests.utils: file /Users/christoph.ludwig/Python/pip-tools/tests/utils.py

[ ... many more metadata refresh messages  ... ]

LOG:  Metadata fresh for packaging._elffile: file /Users/christoph.ludwig/.cache/pre-commit/repo3zt33lob/py_env-python3.11/lib/python3.11/site-packages/packaging/_elffile.py
LOG:  Loaded graph with 249 nodes (0.058 sec)
LOG:  Found 149 SCCs; largest has 39 nodes
LOG:  Processing 148 queued fresh SCCs
LOG:  Processing SCC singleton (tests.test_cli_compile) as inherently stale
tests/test_cli_compile.py:2874: error: Function is missing a type annotation for one or more arguments  [no-untyped-def]
LOG:  Deleting tests.test_cli_compile tests/test_cli_compile.py tests/test_cli_compile.meta.json tests/test_cli_compile.data.json
LOG:  No fresh SCCs left in queue
LOG:  Build finished in 0.253 seconds with 249 modules, and 1 errors
Found 1 error in 1 file (checked 1 source file)

[chrysle]

pre-commit rejected this commit, here's the relevant output:

I guess that's unrelated; it's just rebuilding its cache.
Could you try if setting disallow_incomplete_defs to false in the mypy override works for you? I have the strong feeling that could be the culprit.

[chludwig-haufe]

Could you try if setting disallow_incomplete_defs to false in the mypy override works for you? I have the strong feeling that could be the culprit.

Right, with disallow_incomplete_defs = false in the test override, mypy / pre-commit let the commit pass.

What do you want me to do about the annotations (and, in particular, the protocol classes) I added to the tests? Should I take them out again (or, maybe, move the protocol classes into a dedicated module, because they don't really fit into utils)?

[chrysle]

What do you want me to do about the annotations (and, in particular, the protocol classes) I added to the tests? Should I take them out again

I think that would be the better solution, since it's otherwise unnecessary complex.

[chludwig-haufe]

I think that would be the better solution, since it's otherwise unnecessary complex.

Done.

[chrysle]

Thank you!