Re resolve combined ireq

[zyxue]

An idea for fixing #1511

It will affects this line https://github.com/pypa/pip/blob/fc6d7778a5ee5616cc517ba8b9d41ddf5f204e5c/src/pip/_internal/resolution/legacy/resolver.py#L373 and leads to resolution of the dependencies of ray[default,tune]==1.4.0 properly

Currently, this line evaluated to True because combined_ireq copied the .prepared=True from ray[default]==1.4.0, which was already prepared before.

[atugushev]

Neat! Could you add an integration test to test_cli_compile.py base on the issue details? You could use make_package and make_wheel, see test_duplicate_reqs_combined for example.

[zyxue]

added tests

[zyxue]

@atugushev , any update on this, please?

[richafrank]

It looks like this will conflict with #1507 ! I haven't dug in (I think I tried this solution some time ago, but no longer remember why I abandoned it), so I don't have a recommendation yet, but curious if folks have initial thoughts:

• Including changes from both PRs causes test_combine_different_extras_of_the_same_package to fail.
• Removing prepared = False causes test_combine_install_requirements_for_one_package_with_multiple_extras_reset_prepared to fail as well.
• Removing calls to copy_ireq_dependencies instead causes test_iter_dependencies_after_combine_install_requirements and test_local_duplicate_subdependency_combined to fail.

[zyxue]

Removing prepared = False causes test_combine_install_requirements_for_one_package_with_multiple_extras_reset_prepared to fail as well.

This PR adds two tests, test_combine_install_requirements_for_one_package_with_multiple_extras_reset_prepared is specific to this PR, so it matters less.

But I feel test_combine_different_extras_of_the_same_packageshould pass regardless of which PR is accepted. It tries to prevent regression bug as reported at #1511. This test would also fail in master. if #1507 can make this test pass, you can ignore this PR.

[richafrank]

I'm still looking into this, but I think the issue with the naive combination of the two PRs is that setting prepared = False isn't safe in some cases:

pypi_repository = <piptools.repositories.pypi.PyPIRepository object at 0x10e0f7910>
base_resolver = functools.partial(<class 'piptools.resolver.Resolver'>, cache=<piptools.cache.DependencyCache object at 0x10e177880>)
make_package = <function make_package.<locals>._make_package at 0x10e163af0>, from_line = <function install_req_from_line at 0x10c758430>

    def test_iter_dependencies_after_combine_install_requirements(
        pypi_repository, base_resolver, make_package, from_line
    ):
        res = base_resolver([], repository=pypi_repository)
    
        sub_deps = ["click"]
        package_a = make_package("package-a", install_requires=sub_deps)
        package_b = make_package("package-b", install_requires=["package-a"])
    
        local_package_a = from_line(path_to_url(package_a))
        assert [dep.name for dep in res._iter_dependencies(local_package_a)] == sub_deps
    
        package_a_from_b = from_line("package-a", comes_from=path_to_url(package_b))
        combined = combine_install_requirements(
            pypi_repository, [local_package_a, package_a_from_b]
        )
>       assert [dep.name for dep in res._iter_dependencies(combined)] == sub_deps

tests/test_resolver.py:303: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tests/test_resolver.py:303: in <listcomp>
    assert [dep.name for dep in res._iter_dependencies(combined)] == sub_deps
piptools/resolver.py:427: in _iter_dependencies
    yield from self.repository.get_dependencies(ireq)
piptools/repositories/pypi.py:236: in get_dependencies
    self._dependencies_cache[ireq] = self.resolve_reqs(
piptools/repositories/pypi.py:199: in resolve_reqs
    results = resolver._resolve_one(reqset, ireq)
.tox/py38-pipmain/src/pip/src/pip/_internal/resolution/legacy/resolver.py:379: in _resolve_one
    dist = self._get_dist_for(req_to_install)
.tox/py38-pipmain/src/pip/src/pip/_internal/resolution/legacy/resolver.py:332: in _get_dist_for
    dist = self.preparer.prepare_linked_requirement(req)
.tox/py38-pipmain/src/pip/src/pip/_internal/operations/prepare.py:481: in prepare_linked_requirement
    return self._prepare_linked_requirement(req, parallel_builds)
.tox/py38-pipmain/src/pip/src/pip/_internal/operations/prepare.py:519: in _prepare_linked_requirement
    self._ensure_link_req_src_dir(req, parallel_builds)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <pip._internal.operations.prepare.RequirementPreparer object at 0x10e152970>
req = <InstallRequirement object: package-a==0.1 from file:///private/var/folders/st/jvcp51f52dq9_779qz2lp45h0000gn/T/pytest-of-rich/pytest-188/popen-gw5/test_iter_dependencies_after_c0/packages/package-a/0.1 editable=False>
parallel_builds = False

    def _ensure_link_req_src_dir(
        self, req: InstallRequirement, parallel_builds: bool
    ) -> None:
        """Ensure source_dir of a linked InstallRequirement."""
        # Since source_dir is only set for editable requirements.
        if req.link.is_wheel:
            # We don't need to unpack wheels, so no need for a source
            # directory.
            return
>       assert req.source_dir is None
E       AssertionError

.tox/py38-pipmain/src/pip/src/pip/_internal/operations/prepare.py:333: AssertionError

The new test from the other PR exposes the issue here that trying to prepare a linked requirement a second time fails an assertion. There are many cases that avoid this code path, but unfortunately if the particular requirement hits the code path with the comment

            # None of the optimizations worked, fully prepare the requirement
            return self._prepare_linked_requirement(req, parallel_builds)

then an assert in pip fails. We can try to reset more attributes, but it might be a bit of whack-a-mole. I've started working on an update that creates a new InstallRequirement instance for the combination, instead of the copy we make today. Not done yet, but I recall that this is also where I ended up a while back when I tried prepared = False.

[richafrank]

I forgot to update here: #1519 is intended to resolve the conflict between these two.

[AndydeCleyre]

I'm going to close this in favor of focusing related attention on #1519. Thank you for this, and please let me know if something's amiss!