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

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

Merged
merged 11 commits into from
Feb 25, 2021

Conversation

IceTDrinker
Copy link
Contributor

@IceTDrinker IceTDrinker commented Feb 13, 2021

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
Contributor Author

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

@codecov
Copy link

codecov bot commented Feb 13, 2021

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
Contributor Author

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

Cheers

@atugushev atugushev added bug fix resolver Related to dependency resolver labels Feb 15, 2021
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
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.

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
Contributor Author

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

Do you need a squashed commit for all this ?

@atugushev
Copy link
Member

Do you need a squashed commit for all this ?

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

@IceTDrinker
Copy link
Contributor Author

Thanks for the reviews

Cheers

@atugushev
Copy link
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