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

Extra requirement support (extras_require) #1363

Merged
merged 17 commits into from
Apr 14, 2021

Conversation

orsinium
Copy link
Contributor

@orsinium orsinium commented Mar 23, 2021

Changelog-friendly one-liner: Add --extra to specify extras_require dependencies.

Supports setup.py and anything PEP-517 compatible. Tested for setup.py, setup.cfg, poetry, and flit.

Close #625.

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

@codecov
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #1363 (15c4a7b) into master (6422fcc) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 15c4a7b differs from pull request most recent head 37c75e8. Consider uploading reports for the commit 37c75e8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1363      +/-   ##
==========================================
+ Coverage   99.65%   99.67%   +0.01%     
==========================================
  Files          33       33              
  Lines        2939     3037      +98     
  Branches      308      327      +19     
==========================================
+ Hits         2929     3027      +98     
  Misses          5        5              
  Partials        5        5              
Impacted Files Coverage Δ
piptools/scripts/compile.py 100.00% <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%> (ø)
tests/test_utils.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 6422fcc...37c75e8. Read the comment docs.

@atugushev atugushev added enhancement Improvements to functionality pep-517 Related to PEP-517 standard API labels Mar 23, 2021
@orsinium
Copy link
Contributor Author

orsinium commented Mar 24, 2021

Ready for review. I hope, we'll be able to take it into #1362, it's closely related to #1356, they play nice together.

@atugushev atugushev added this to the 6.1.0 milestone Mar 24, 2021
@atugushev
Copy link
Member

Ready for review. I hope, we'll be able to take it into #1362, it's closely related to #1356, they play nice together.

Ok, I've added it to 6.1.0.

@atugushev atugushev mentioned this pull request Mar 24, 2021
2 tasks
@orsinium
Copy link
Contributor Author

Is there anything that remains to do?

@atugushev
Copy link
Member

Could you add a test for pip-compile --extra=whatever requirements.in?

@orsinium
Copy link
Contributor Author

Could you add a test for pip-compile --extra=whatever requirements.in?

What's the expected behavior? AFAIK, requirements.in doesn't support extras. At the moment, I expect the flag to be ignored in this case. Should we fail instead?

@atugushev
Copy link
Member

@orsinium IMO there should be an error.

@orsinium
Copy link
Contributor Author

Done 👍🏿

Comment on lines +1741 to +1742
small-fake-c = "0.3"
small-fake-d = "0.4"
Copy link
Member

Choose a reason for hiding this comment

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

Is it intentional that these are duplicated in here and in the extras?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is how poetry works. Here is the specification, in extras only names.

Comment on lines +1781 to +1782
assert "small-fake-e" not in out.stderr
assert "small-fake-f" not in out.stderr
Copy link
Member

Choose a reason for hiding this comment

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

I don't fully understand. Is "extra" supposed to only select the dists that are listed in extras but not the main runtime deps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

small-fake-e and small-fake-f are from another extra, the test one, we pass only dev.

"--extra",
"extras",
multiple=True,
help="names of extras_require to install",
Copy link
Member

Choose a reason for hiding this comment

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

  1. s/n/N
  2. If this is supposed to install only extras but not the default runtime deps, can this be renamed into --extra-only?
  3. If it's supposed to merge those extras with the main runtime deps list, can we add an additional flag --extras-only/--exclude-default-deps or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last one, default dependencies are always selected. I don't think --exclude-default-deps makes sense, I'd make a separate discussion for it. AFAIK, setuptools/poetry/flit/pip doesn't have a way to opt-out installing the default dependencies, so I see no reason why pip-tools should.

Copy link
Member

Choose a reason for hiding this comment

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

It's actually a long-standing feature request in pip and I think there's been no opposition, just nobody had time to come up with an implementation. The main motivation was that people often supply test/dev/docs extras for different processes they have and some of them don't really require the dist itself to be installed. Of course, there's another camp saying that this sort of deps should be put elsewhere because they aren't necessary for the runtime and there should be env pins anyway.
But if pip-tools were to implement this flag, it'd facilitate a hybrid of these two approaches with the direct open-ended deps in the dist metadata configs and the pins existing in separate constraints files.
But yes, this could probably be brought up in a separate discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm from that camp too. I have a separate environment for every tool. However, most of the tools still need dependencies: pytest for sure, mypy for type-checking of third-party libs usage, pylint for inference-based tasks. In my projects, only flake8 works nicely without knowing the project dependencies. And I keep a separate requirements.txt for it. Yep, makes the root of the project a bit messier but works. This is why I made dephell a long time ago, to handle all this zoo :)

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

I think it's merge-ready, any other comments can be addressed separately.

@webknjaz webknjaz self-requested a review March 31, 2021 18:57
@webknjaz
Copy link
Member

Also:

The "extra" variable is special. It is used by wheels to signal which specifications apply to a given extra in the wheel METADATA file, but since the METADATA file is based on a draft version of PEP-426, there is no current specification for this. Regardless, outside of a context where this special handling is taking place, the "extra" variable should result in an error like all other unknown variables.

@atugushev
Copy link
Member

atugushev commented Apr 1, 2021

@webknjaz thanks for the info! Perhaps, "extra" should be filtered/stripped out in format_requirement somehow. Ideas?

@orsinium
Copy link
Contributor Author

orsinium commented Apr 2, 2021

Possible solutions I see for removing extra marker:

  1. Hacking with regexp
  2. Hacking through inspection of packaging.Marker._marker. It has an interesting structure of tuples in lists inside. If we assume that extra always comes at the top level, we can easily find and remove it. That means getting into a "private" method but packaging should be very stable and unlikely to change.
  3. https://github.com/dephell/dephell_markers has .remove method. I made it a long time ago for dephell, its well well tested and works. Still, a third-party dependency for a small task.

I think, the second option should be fine 🤔

@atugushev
Copy link
Member

Approach №2 is fine by me 👍🏻

@orsinium
Copy link
Contributor Author

orsinium commented Apr 6, 2021

I've implemented and integrated drop_extras function which removes extra from markers. Unit tests are included and all seems to be working fine. However, I can't cover one line somehow 🤔

@orsinium
Copy link
Contributor Author

orsinium commented Apr 6, 2021

Huh. I've put a breakpoint there and it IS covered, for sure. I have a deja vu. Coverage.py seems to be quite flaky about branch coverage for if conditions. I definitely faced something like this in deal but I don't remember the exact solution. Probably, I've just rewritten the condition back then.

@orsinium
Copy link
Contributor Author

orsinium commented Apr 6, 2021

I decided to hack it around here too 🙃

@ssbarnea
Copy link
Member

@atugushev @orsinium @webknjaz Is there anything blocking progress on this PR? I do find it very useful and a blocker for the new release.

@orsinium
Copy link
Contributor Author

Nothing from my side. There is a new logic (3b63aa9) that needs a review from maintainers.

@atugushev
Copy link
Member

Hello @ssbarnea! Thanks for pinging. I'm going to look into this shortly.

@ssbarnea
Copy link
Member

I should be the owe thanking you. That is indeed a very important feature, one that prevented further adoption from more people.

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.

@orsinium thanks for fixing the bug with extras 🙏🏻 The PR's all good!

@atugushev atugushev merged commit 27b62c3 into jazzband:master Apr 14, 2021
atugushev added a commit to atugushev/pip-tools that referenced this pull request Apr 14, 2021
@atugushev
Copy link
Member

pip-tools v6.1.0 is released 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to functionality pep-517 Related to PEP-517 standard API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pip-compile ignores extras_require when parsing setup.py
4 participants