Skip to content

Consider specifiers for equality operator to pin a dependency in make_install_requirement - #1323

Merged
atugushev merged 11 commits into
jazzband:masterfrom
IceTDrinker:master
Feb 25, 2021
Merged

atugushev merged 11 commits into
jazzband:masterfrom
IceTDrinker:master

Conversation

@IceTDrinker

@IceTDrinker IceTDrinker commented Feb 13, 2021 •

Copy link
Copy Markdown
Contributor

Resolves #1114

Haven't written the test yet, as I would like to know if the spirit of the change suits the authors first
Tried to run tox -e checkqa as proposed in contribution guidelines but had a failure with github auth

  • Changed function signature to take ireq directly rather than passing its
    members one by one
  • Changed type hint for version as I was getting a pip Version rather than
    a string, maybe there are some parts of the code calling with string though
  • Can confirm it generates a pinned torch with === in my use case as
    expected

Changelog-friendly one-liner: Prefer === over == when generating requirements.txt if a dependency was pinned with ===

Contributor checklist
  • Provided the tests for the changes.
  • Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@IceTDrinker

Copy link
Copy Markdown
Contributor Author

Solved the tox auth issue, will fix the issues on my end and understand what happens with the failing test cases

@codecov

codecov Bot commented Feb 13, 2021 •

Copy link
Copy Markdown

Codecov Report

Merging #1323 (8338272) into master (1931e21) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1323   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files          33       33           
  Lines        2928     2949   +21     
  Branches      318      322    +4     
=======================================
+ Hits         2917     2938   +21     
  Misses          5        5           
  Partials        6        6           
Impacted Files Coverage Δ
piptools/repositories/pypi.py 97.25% <ø> (-0.02%) ⬇️
piptools/repositories/local.py 96.22% <100.00%> (ø)
piptools/utils.py 100.00% <100.00%> (ø)
tests/conftest.py 100.00% <100.00%> (ø)
tests/test_cli_compile.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1931e21...8338272. Read the comment docs.

@IceTDrinker

Copy link
Copy Markdown
Contributor Author

Feedback welcome, squash is ready locally (removed the pytest network marker that's not required by my test anymore)

Cheers

Comment thread tests/test_cli_compile.py Outdated
@atugushev atugushev added bug fix resolver Related to dependency resolver labels Feb 15, 2021
Comment thread tests/test_cli_compile.py Outdated
make_install_requirement

- Changed function signature to take ireq directly rather than passing its
members one by one
- Changed type hint for version as I was getting a pip Version rather than
a string, maybe there are some parts of the code calling with string though
- Can confirm it generates a pinned torch with === in my use case as
expected
…t from

a specifier version as it made tests fail
- Use case example where torch has multiple pin sources

@atugushev atugushev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍🏼 Thanks, @IceTDrinker, for your patience and great work!

@atugushev atugushev added this to the 6.0.0 milestone Feb 24, 2021
@IceTDrinker

Copy link
Copy Markdown
Contributor Author

LGTM 👍🏼 Thanks, @IceTDrinker, for your patience and great work!

Do you need a squashed commit for all this ?

@atugushev

Copy link
Copy Markdown
Member

Do you need a squashed commit for all this ?

I'll use the "Squash and merge" button, no worries 👍🏼

@IceTDrinker

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews

Cheers

@atugushev

Copy link
Copy Markdown
Member

pip-tools v6.0.0 is released 🚀

This was referenced Mar 15, 2021
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.

Possible bug: problems resolving pytorch (cuda).

2 participants