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

Improve handling of extras #6130

Conversation

maksbotan
Copy link
Contributor

@maksbotan maksbotan commented Aug 7, 2022

Pull Request Check List

This changeset improves caching of dependencies with extras in two ways:

  • foo[extra] -> foo dependency preserves information about foo's source. If it comes from secondary repo, poetry will not look for it on PyPI
  • Do not resolve optional dependencies that are not requested: if we depend on foo[extra1], do not look into dependencies of foo[extra2]. This will save time when extra2 contains some heavy-to-resolve dependencies like VCS ones.
  • Added tests for changed code.
  • Updated documentation for changed code.

I'm all in for adding tests, but I'd need guidance how to proceed.

When this PR is merged, I'll backport it to 1.1 branch as well.

@maksbotan
Copy link
Contributor Author

@radoering hi! I've also updated this one, now tests pass.

@maksbotan
Copy link
Contributor Author

I will add a test for first change — check that poetry does not try to query PyPI for locked package with extras and secondary source.

However I'm a bit at lost about the second one. What is the cleanest way to test that change?

@LarsDu
Copy link

LarsDu commented Sep 9, 2022

Does this PR resolve the following issue?
#6419

@maksbotan
Copy link
Contributor Author

@LarsDu do you have reproduction steps for that issue? I can check if my branch fixes it.

@radoering
Copy link
Member

Maybe, we can split this in two PRs? The first change seems quite clear and low risk (and easier to test). The second one has more potential for negative side effects. We should be able to proceed quite fast with the first one but I probably need some time to think about the second one.

@maksbotan
Copy link
Contributor Author

@radoering no problem, I'll split
What are your thoughts about required test cases?

@radoering
Copy link
Member

I have not yet an idea for the second one. For the first one something like this might be sufficient (test_provider.py):

@pytest.mark.parametrize("source_name", [None, "repo"])
def test_complete_package_with_extras_preserves_source_name(
    provider: Provider, repository: Repository, source_name: str | None
) -> None:
    package_a = Package("A", "1.0")
    package_b = Package("B", "1.0")
    dep = get_dependency("B", "^1.0", optional=True)
    package_a.add_dependency(dep)
    package_a.extras = {"foo": [dep]}
    repository.add_package(package_a)
    repository.add_package(package_b)

    dependency = Dependency("A", "1.0", extras=["foo"])
    if source_name:
        dependency.source_name = source_name

    complete_package = provider.complete_package(
        DependencyPackage(dependency, package_a)
    )

    requires = complete_package.package.all_requires
    assert len(requires) == 2
    assert requires[0].name == "a"
    assert requires[0].source_name == source_name
    assert requires[1].name == "b"
    assert requires[1].source_name is None

@maksbotan
Copy link
Contributor Author

@radoering I've split the first change into #6472, along with your test.

neersighted pushed a commit that referenced this pull request Sep 11, 2022
This PR splits out simple part of #6130. Once this merged I'll update
that PR to contain only second (more controversial) fix.
@LarsDu
Copy link

LarsDu commented Sep 11, 2022

@maksbotan I'll look into seeing if this branch resolves the issue sometime this week. We are primarily concerned with case 2:

When we have multiple teams working out of the same repo with different extra dependencies, we want to avoid having the solver run across all extras as dependency conflicts can become extremely problematic.

Having different groups of extras will necessitate making unit tests aware of dependencies, but that is a reasonable tradeoff.

@radoering I believe this may cause side-effects as virtually all groups are currently having poetry solve across the union of all base + extras dependencies?
Perhaps making this behavior optional would be the way forward?

@neersighted
Copy link
Member

neersighted commented Sep 11, 2022

This PR solves a different issue from what you are interested in @LarsDu -- the idea here is to not solve for extras that are not defined on a dependency of your project.

If your project (the 'root package') defines multiple extras, they must not conflict and the solver will ensure that all extras are installable at the same time. This is currently how Poetry's solver works and you are expressing a feature request found at #6419.

A change to this behavior would be a large-scale, invasive refactor, and so far there have been no proposals on how to do so in a backwards-compatible and flexible manner.

@maksbotan
Copy link
Contributor Author

@radoering I've updated this PR to include only the second one of my changes.

However, my initial pain point is solved by my other PR — the one for reusing locked git repo dependency. Previously poetry used to refetch git repo that is a transitive dependency under extra I don't actually depend on. Now that refetching is gone, this one is not so important for me.

What do you think about the change in general?

@radoering
Copy link
Member

@maksbotan Finally, I had the time to take a closer look. I think I found a simpler solution to solve this issue. See #6615.

@radoering
Copy link
Member

First part split out to #6472, second part superseded by #6615.

@radoering radoering closed this Sep 24, 2022
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants