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

Jump to bottom

Instantiate a new accumulator InstallRequirement for combine_install_requirements output #1519

Merged
merged 11 commits into from May 31, 2022
Merged

Instantiate a new accumulator InstallRequirement for combine_install_requirements output #1519

merged 11 commits into from May 31, 2022
+245 −88

Conversation

richafrank
Copy link
Contributor

@richafrank richafrank commented Oct 28, 2021 •
edited by atugushev

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).

richafrank
richafrank commented Oct 28, 2021
View reviewed changes
tests/test_resolver.py Outdated Show resolved Hide resolved
richafrank
richafrank commented Oct 28, 2021
View reviewed changes
piptools/resolver.py
# 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.

This was referenced Oct 29, 2021
Open
Closed
@richafrank richafrank force-pushed the combine-without-copy branch 2 times, most recently from bd3caf8 to b7e5d16 Compare November 5, 2021 15:42
@richafrank richafrank mentioned this pull request Dec 18, 2021
Closed
@richafrank
Copy link
Contributor Author

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

@richafrank richafrank added bug fix resolver Related to dependency resolver labels Jan 8, 2022
This was referenced Feb 11, 2022
Closed
Closed
Closed
@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.

@AndydeCleyre AndydeCleyre mentioned this pull request Feb 26, 2022
Closed
RedRoserade added a commit to RedRoserade/pip-tools-repro-repo-a that referenced this pull request Feb 26, 2022
@RedRoserade
Test jazzband/pip-tools#1519
80a89d6
@Jorricks
Copy link

Jorricks commented Mar 11, 2022 •
edited

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.

@AndydeCleyre AndydeCleyre mentioned this pull request Mar 17, 2022
Closed
@AndydeCleyre AndydeCleyre mentioned this pull request Mar 31, 2022
Closed
@frejonb
Copy link

frejonb commented Apr 4, 2022 •
edited

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

@richafrank
Partially revert "Fix a regression where multiple declarations of a d…
828e2d6 
…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.
@richafrank
Failing test for case of resolver combining extras after the first
46ec202 
round of resolution.

By this point, we've frozen the dependencies.
@richafrank richafrank mentioned this pull request May 30, 2022
Merged
4 tasks
atugushev
atugushev approved these changes May 30, 2022
View reviewed changes
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 👍🏻

@richafrank
Test the entire resolver output
97471ea 
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
@richafrank richafrank mentioned this pull request Jun 1, 2022
Closed
@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
Reviewers

@zyxue zyxue zyxue left review comments

@atugushev atugushev atugushev approved these changes

Assignees
No one assigned
Labels
resolver Related to dependency resolver
Projects
None yet
Milestone
6.7.0
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
@richafrank @mmerickel @Jorricks @frejonb @atzannes @zyxue @atugushev @AndydeCleyre