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

Instantiate a new accumulator InstallRequirement for combine_install_requirements output #1519

Merged
merged 11 commits into from
May 31, 2022

Conversation

richafrank
Copy link
Contributor

@richafrank richafrank commented Oct 28, 2021

Replace the copy+mutate behavior of combine_install_requirements with instantiation of a new InstallRequirement. Since the PyPIRepository caches dependencies keyed by InstallRequirement instances, we no longer need to worry about:

  • preparing to force discovery of the ireq's name, then needing to keep track of the the "prepared" copy's dependencies (via the copy_ireq_dependencies method)
  • preparing an ireq for its name, then copying and mutating and needing to get dependencies anew for the mutated copy, which has already been prepared, and it's not really safe to prepare again
  • avoiding the previous situation by preparing a copy of the ireq for its name (the "name_proxy")

i.e. a much simpler solution. We can prepare an ireq to get its name and not worry about later uses.

The inputs to the new instance include the link, as done in #1506, which solves the local file information loss.

Builds on and replaces #1507, #1385, and #1512. The tests from all PRs pass, including the xfail I added in #1507.

Fixes #1505 , #1511 , #1054

Contributor checklist
  • Provided the tests for the changes.
  • Assure PR title is short, clear, and good to be included in the user-oriented changelog
Maintainer checklist
  • Assure 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).

tests/test_resolver.py Outdated Show resolved Hide resolved
# Don't just yield from above. Instead, use the same `markers`-stripping
# behavior as we have for cached dependencies below.
dependency_strings = sorted(str(ireq.req) for ireq in dependencies)
yield from self._ireqs_of_dependencies(ireq, dependency_strings)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this change to make this behavior consistent with the cached dependencies case, tests failed because the output included markers, e.g. fake-tensorboardx==0.5; extra == "tune".

@richafrank
Copy link
Contributor Author

This was necessary to solve the "conflict" described in #1512 (comment), but also feels like the longer-term better solution.

@richafrank
Copy link
Contributor Author

@atugushev @AndydeCleyre Thoughts on timing for a review? Looks like not much activity across the repo since October...

@mmerickel
Copy link

Kind of in a pickle right now stuck on 6.2.0 due to this issue but that version is busted with new pip's fancy new log handler. Several new versions appear to have shipped with this regression so it'd be great to see some movement on this. To help I can confirm I ran this PR on my repo with 30 local packages using the -e file:src/foo#egg=foo syntax in them and the only changes observed is that it normalized packages with . in the name to a -. For example zope.sqlalchemy is now zope-sqlalchemy in the requirements.txt but this is a valid name for the package. Without this fix in 6.5.1 I see many dependencies dropped from the resulting txt file as well as many references to local packages removed from dependencies that are kept.

RedRoserade added a commit to RedRoserade/pip-tools-repro-repo-a that referenced this pull request Feb 26, 2022
@Jorricks
Copy link

Jorricks commented Mar 11, 2022

First of all, @richafrank this PR looks in very good shape. Nice work!
Good comments as well :)

Kind of in a pickle right now stuck on 6.2.0 due to this issue but that version is busted with new pip's fancy new log handler. Several new versions appear to have shipped with this regression so it'd be great to see some movement on this. To help I can confirm I ran this PR on my repo with 30 local packages using the -e file:src/foo#egg=foo syntax in them and the only changes observed is that it normalized packages with . in the name to a -. For example zope.sqlalchemy is now zope-sqlalchemy in the requirements.txt but this is a valid name for the package. Without this fix in 6.5.1 I see many dependencies dropped from the resulting txt file as well as many references to local packages removed from dependencies that are kept.

I'd actually argue that that is not a valid package name. If you lookup pypi, there is no zope-sqlalchemy, there is only zope.sqlalchemy. I actually just closed a ticket over at zope which truly should be opened here: zopefoundation/zope.interface#255. I will open an issue here as that is not related to this PR and should not block this PR in any way.

@frejonb
Copy link

frejonb commented Apr 4, 2022

Thanks for this PR. I'm extremely blocked by #1505. I'm not really sure what is missing for approving this PR, but would it be possible that there is a shorter, easier to review changes that only address #1505?

@atzannes
Copy link

I think I speak on behalf of many when I say that I am rooting this PR to get merged and released ASAP

…ependency"

(commit d720fe6)

This brings back copy_ireq_dependencies and combines its use with that of the
proxies-for-naming. Editable and url_requirement ireqs are not cached by the
Repository, so when dependencies are queried for them once, no dependencies
will be found later for the copy made by combine_install_requirements.

copy_ireq_dependencies links those dependencies at the time of copy.
round of resolution.

By this point, we've frozen the dependencies.
Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get that in 👍🏻

to ensure nothing else was emitted
@atugushev atugushev merged commit e2d0fae into jazzband:master May 31, 2022
@richafrank richafrank deleted the combine-without-copy branch May 31, 2022 23:58
@atugushev atugushev changed the title Instantiate a new accumulator InstallRequirement for combine_install_requirements output Instantiate a new accumulator InstallRequirement for combine_install_requirements output Jun 7, 2022
@atugushev atugushev added this to the 6.7.0 milestone Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolver Related to dependency resolver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Second level dependencies missing on for local file packages on >= 6.3.1
8 participants