Annotate primary requirements and VCS dependencies #1058

AndydeCleyre · 2020-02-07T20:24:18Z

Resolves #881
Resolves #293

This change brings annotations to primary requirements in the compilation output.

The annotation may be merely a reqs-in source:

django-debug-toolbar==2.2  # via -r requirements.in (line 2)

or it may additionally include reverse dependencies:

django==3.0.3             # via -r requirements.in (line 1), django-debug-toolbar

Existing tests are modified to either adjust their expectations, or compile with --no-annotations if annotations are irrelevant. Two tests have been inverted and renamed:

-test_format_requirement_not_for_primary
+test_format_requirement_for_primary
-test_format_requirement_not_for_primary_lower_case
+test_format_requirement_for_primary_lower_case

Changelog-friendly one-liner: Primary requirements and VCS dependencies now get annotated with any source .in files and reverse dependencies

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

Please review these changes with the following questions in mind:

Is the new annotation string as desired?
Is it desirable to add an option to disable this new behavior?
Should the non-annotation-focused tests needing modification be, generally, expecting an exact annotation? Or should we simply use --no-annotate?
In which cases might an InstallRequirement's comes_from attribute be a str, or an InstallRequirement? The following alternatives seem to result in the same output. Which is saner or preferred, or handles potential edge cases better?

 required_by |= {
     src_ireq.comes_from
-    for src_ireq in ireq._source_ireqs
     if isinstance(src_ireq.comes_from, str)
+    else src_ireq.comes_from.name.lower()
+    for src_ireq in ireq._source_ireqs
+    if src_ireq.comes_from
 }

code in context

If/when all looks good, want it squashed?

codecov · 2020-02-07T20:32:31Z

Codecov Report

Merging #1058 into master will increase coverage by 0.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1058      +/-   ##
==========================================
+ Coverage   99.35%   99.47%   +0.12%     
==========================================
  Files          34       34              
  Lines        2463     2476      +13     
  Branches      307      312       +5     
==========================================
+ Hits         2447     2463      +16     
+ Misses          8        6       -2     
+ Partials        8        7       -1

Impacted Files	Coverage Δ
piptools/repositories/pypi.py	`95.16% <0.00%> (+1.44%)`	⬆️

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 f2e2e2b...ca69a92. Read the comment docs.

graingert · 2020-02-07T21:00:09Z

The output looks great! I don't think this should introduce a new setting though

atugushev

The idea and approach are awesome! I've suggested some improvements below:

piptools/writer.py

atugushev · 2020-02-08T03:31:52Z

@AndydeCleyre

Is the new annotation string as desired?

It looks great! 👍

Is it desirable to add an option to disable this new behavior?

This improvement is pretty useful, no need to disable it. Otherwise --no-annotate helps.

Should the non-annotation-focused tests needing modification be, generally, expecting an exact annotation? Or should we simply use --no-annotate?

I think it's okay to disable annotations to focus on certain things desired to test.

In which cases might an InstallRequirement's comes_from attribute be a str, or an InstallRequirement? The following alternatives seem to result in the same output. Which is saner or preferred, or handles potential edge cases better?

I'd prefer the same way as pip does.

If/when all looks good, want it squashed?

Yes, better to squash.

atugushev · 2020-02-08T07:19:15Z

@AndydeCleyre

4. In which cases might an InstallRequirement's comes_from attribute be a str, or an InstallRequirement?

Usually, if a requirement came from a nested requirement file (included via -r), see related code in pip.

tests/test_cli_compile.py

atugushev · 2020-02-11T17:05:17Z

@AndydeCleyre

I've noticed that pip-compile setup.py outputs temporary file name. See:

$ pip-compile setup.py
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile setup.py
#
click==7.0                # via -r /var/folders/4x/rz_w89dx1z3bydfd6qpdjn_h0000gn/T/tmp10jejvdm (line 1)
six==1.14.0               # via -r /var/folders/4x/rz_w89dx1z3bydfd6qpdjn_h0000gn/T/tmp10jejvdm (line 2)

Have no idea how to fix this yet 🤔

atugushev · 2020-02-11T17:13:42Z

Probably, we could enrich those constraints somehow with ireq.comes_from = dist.get_name()

pip-tools/piptools/scripts/compile.py

Lines 334 to 341 in d405bf9

    
           constraints.extend( 
        
               parse_requirements( 
        
                   tmpfile.name, 
        
                   finder=repository.finder, 
        
                   session=repository.session, 
        
                   options=repository.options, 
        
               ) 
        
           )

AndydeCleyre · 2020-02-11T17:14:42Z

Thank you @atugushev, good catch. I'll look into this today.

AndydeCleyre · 2020-02-11T17:31:52Z

My proposed solution will also handle reqin as stdin.

atugushev · 2020-02-11T17:46:00Z

@AndydeCleyre

My proposed solution will also handle reqin as stdin.

Which one?

For the stdin it could be ireq.comes_from = "-r /dev/stdin", but would look strange for windows 😄

graingert · 2020-02-11T17:55:25Z

-r -?

AndydeCleyre · 2020-02-11T19:48:22Z

Which one?

Sorry, I meant "my proposal [which will exist] will . . ."

-r - as @graingert suggests looks good to me for stdin.

For setup.py, yes dist.get_name() should be included in the string, as well as setup.py. It should also not be ambiguous with any repo package names. Some options for the format (supposing dist.get_name() == 'pip-tools'):

-r setup.py (pip-tools)
-r setup.py: pip-tools
pip-tools setup.py
pip-tools: setup.py
pip-tools (setup.py)
setup.py (pip-tools)

I'm leaning toward 5: <pkgname> (setup.py) as it seems to most closely match existing annotations; if <pkgname> is included, then it is similar to listing an external dependency, with some more detail. But 1 is also very logical, and 6 has some clear merit to me as well.

Which looks best to you, @atugushev and @graingert ?

New questions though:

I thought that setup.py should be rel/path/to/setup.py if it's not adjacent to the output file. But then I realized the relpaths found in the output for -r and -c specified reqsins are only relative to the target reqsin file (probably simply copied from the target reqsin), regardless of the location of the output file.

Should any setup.py annotations list relpath (if not adjacent) to the output file? I think so.
Should -r and -c relpaths be calculated and annotated based on output location rather than input location? I would think so.
If 2, then should that be part if this PR, which can be taken as an "annotation consistency" attempt?
If 2 and/or 1 and output file is set to stdout, relpath should be anchored to the user's working dir, yes?

graingert · 2020-02-11T19:55:31Z

sometimes in a .in file I'll refer to another .txt file eg:

# requirements.in
ham
spam

# requirements-test.in
-r ./requirements.txt
pytest-anyio

What would the outputs look like?

AndydeCleyre · 2020-02-11T20:47:46Z

@graingert

With the current state of this PR:

# requirements.in
requests
structlog

# requirements-test.in
-r ./requirements.txt
pytest

If no txts exist yet, pip-compile requirements-test.in will fail. Otherwise:

$ pip-compile --no-header requirements.in
certifi==2019.11.28       # via requests
chardet==3.0.4            # via requests
idna==2.8                 # via requests
requests==2.22.0          # via -r requirements.in (line 2)
six==1.14.0               # via structlog
structlog==20.1.0         # via -r requirements.in (line 3)
urllib3==1.25.8           # via requests
$ pip-compile --no-header requirements-test.in
attrs==19.3.0             # via pytest
certifi==2019.11.28       # via -r ./requirements.txt (line 1), requests
chardet==3.0.4            # via -r ./requirements.txt (line 2), requests
idna==2.8                 # via -r ./requirements.txt (line 3), requests
more-itertools==8.2.0     # via pytest
packaging==20.1           # via pytest
pluggy==0.13.1            # via pytest
py==1.8.1                 # via pytest
pyparsing==2.4.6          # via packaging
pytest==5.3.5             # via -r requirements-test.in (line 3)
requests==2.22.0          # via -r ./requirements.txt (line 4)
six==1.14.0               # via -r ./requirements.txt (line 5), packaging, structlog
structlog==20.1.0         # via -r ./requirements.txt (line 6)
urllib3==1.25.8           # via -r ./requirements.txt (line 7), requests
wcwidth==0.1.8            # via pytest

Note that if the files are adjacent, including ./ in the .in file is unnecessary, and if omitted will also be omitted from the output.

Answers to my "new questions" above would change the relpaths in the output in the case where --output-file is used to put it in some other folder.

graingert · 2020-02-11T21:18:23Z

Looks very nice

AndydeCleyre · 2020-02-11T21:21:57Z

@graingert Do you have a clear preference for either the setup.py annotation format, or if the annotation relpaths should all be based on the output location rather than the input location?

graingert · 2020-02-11T21:28:01Z

I think the annotation paths should be based on output location, I don't use setup.py maybe you could use file://../path/to/project/dir

AndydeCleyre · 2020-02-11T21:42:51Z

I'm noticing the path handling is a little sloppy here:

$ mkdir some.folder
$ echo 'requests' > some.folder/reqsin
$ pip-compile --no-header some.folder/reqsin
$ ls some.folder
reqsin
$ cat some.txt
certifi==2019.11.28       # via requests
chardet==3.0.4            # via requests
idna==2.8                 # via requests
requests==2.22.0          # via -r some.folder/reqsin (line 1)
urllib3==1.25.8           # via requests

graingert · 2020-02-11T21:46:13Z

I worry the line numbers might cause too much diff noise when depending on compiled requirements.txt

…

On Tue, 11 Feb 2020, 21:42 Andy Kluger, ***@***.***> wrote: I'm noticing the path handling is a little sloppy here <https://github.com/jazzband/pip-tools/blob/d405bf9c35e9fd37439e33b1f93a708d299bf329/piptools/scripts/compile.py#L243> : $ mkdir some.folder $ echo 'requests' > some.folder/reqsin $ pip-compile --no-header some.folder/reqsin $ ls some.folderreqsin $ cat some.txtcertifi==2019.11.28 # via requestschardet==3.0.4 # via requestsidna==2.8 # via requestsrequests==2.22.0 # via -r some.folder/reqsin (line 1)urllib3==1.25.8 # via requests — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1058?email_source=notifications&email_token=AADFATCXPTSDWDJCD3HFGV3RCMLVZA5CNFSM4KRUM3AKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELOF7NQ#issuecomment-584867766>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADFATEVOEFYPCLKTKEANJDRCMLVZANCNFSM4KRUM3AA> .

tests/test_cli_compile.py

atugushev · 2020-02-12T20:04:10Z

@atugushev Should I open a discussion issue for general path handling improvements, covering at least annotations and compilation output path determination?

Please go for it.

atugushev

Awesome work! 🎉 🎉 🎉

piptools/scripts/compile.py

graingert · 2020-02-18T11:38:25Z

@atugushev any chance for a release soon with this new feature?

AndydeCleyre · 2020-02-18T23:00:58Z

I thought I'd done a final squash already, but I hadn't, so here it is. No code changes since the str.format change up here.

atugushev · 2020-02-19T18:30:50Z

@AndydeCleyre could you please fix the QA issues?

…y and VCS reqs; fixes jazzband#881 and jazzband#293 Change Notes ------------ - Annotations now include reqsin/setup.py/stdin info when relevant - Always send reverse_dependencies to writer._format_requirement as a dict, even if empty - Invert checks for primary requirements not getting annotations, as they now do - Bypass annotation noise in tests which don't target annotations anyway, by passing --no-annotate - ireqs returned by get_best_match now have the parameter ireq's _source_ireqs, if any - reverse_dependencies are no longer passed around independently outside the resolver/cache, but rather determined from an ireq's comes_from and _source_ireqs - primary_packages are no longer passed around independently, but rather determined by an ireq's constraint - In tests, reverse dependencies for fake ireqs are now specified by setting the ireq's comes_from - These changes do _not_ improve handling of relative paths to reqsin files in annotations (see jazzband#1067) Co-authored-by: Albert Tugushev <albert@tugushev.ru>

AndydeCleyre · 2020-02-19T18:43:57Z

@atugushev whoops, didn't run tox after using the github interface to merge in the format string. Should be good when the tests finish now.

atugushev · 2020-02-19T18:46:10Z

@AndydeCleyre thanks!

@graingert I'll try to release within 48 hrs.

atugushev · 2020-02-20T17:13:03Z

pip-tools v4.5.0 is released.

gsakkis · 2020-02-21T15:47:44Z

OK, still looking for feedback on whether better path handling should be in a later PR, and on the current output:
$ echo -e 'requests\nstructlog' > requirements.in
$ pip-compile --no-header - <requirements.in -o requirements.txt
certifi==2019.11.28       # via requests
chardet==3.0.4            # via requests
idna==2.8                 # via requests
requests==2.22.0          # via -r -
six==1.14.0               # via structlog
structlog==20.1.0         # via -r -
urllib3==1.25.8           # via requests
$ pip-compile --no-header setup.py
click==7.0                # via pip-tools (setup.py)
six==1.14.0               # via pip-tools (setup.py)
If that output looks good to everyone, I'll get it covered by a test. Note that no line numbers are specified -- are they desired? It's not like a user will be able to cross reference it with a real file.

Lines numbers apparently were included in v4.5.0. FWIW I just run it over a large requirement set and as @graingert commented the diff noise is excessive even for small changes in requirements.in.

jmbowman · 2020-02-21T16:30:23Z

I agree the diff noise is irritating, but the line numbers did just prove useful also:

mock==3.0.5               # via -c requirements/edx/../constraints.txt (line 70), -c requirements/edx/../constraints.txt (line 76), -r requirements/edx/paver.in (line 17)

Our constraints file had duplicate restrictions on the mock version added by separate developers in different PRs, had already gone 2 weeks without anybody noticing until this change highlighted it.

AndydeCleyre · 2020-02-21T18:00:36Z

@gsakkis

My comment about there being no line numbers was only about annotations for stdin and setup.py -- are you getting line numbers for those?

graingert · 2020-02-21T18:13:59Z

@jmbowman

I agree the diff noise is irritating, but the line numbers did just prove useful also:
mock==3.0.5               # via -c requirements/edx/../constraints.txt (line 70), -c requirements/edx/../constraints.txt (line 76), -r requirements/edx/paver.in (line 17)
Our constraints file had duplicate restrictions on the mock version added by separate developers in different PRs, had already gone 2 weeks without anybody noticing until this change highlighted it.

Isn't this illegal in a requirements.txt? This should throw an error

graingert · 2020-02-21T18:15:02Z

I think there should be no line numbers in the generated requirements.txt, and duplicate constraints in a requirements.in considered an error (as it is in pip (I think))

AndydeCleyre · 2020-02-21T18:40:20Z

@graingert

Would you open a new issue for discussion?

jmbowman · 2020-02-21T20:55:44Z

The duplicates were in a constraints file pulled in via -c constraints.txt, which pip (or at least pip-compile) is apparently fine with.

timmc-edx · 2020-02-25T22:01:05Z

FYI: @adamchainz has opened an issue to discuss this: #1074 (includes a PR)

The diff is so big due to jazzband/pip-tools#1058.

AndydeCleyre changed the title ~~Annotate primary requirements~~ Annotate primary requirements; fixes #881 Feb 7, 2020

AndydeCleyre force-pushed the bugfix/881 branch from ab96094 to 63228d1 Compare February 7, 2020 20:37

AndydeCleyre mentioned this pull request Feb 7, 2020

show via for direct deps #881

Closed

AndydeCleyre requested a review from atugushev February 7, 2020 21:11

atugushev reviewed Feb 8, 2020

View reviewed changes

piptools/writer.py Outdated Show resolved Hide resolved

atugushev changed the title ~~Annotate primary requirements; fixes #881~~ Annotate primary requirements Feb 8, 2020

atugushev added the enhancement Improvements to functionality label Feb 8, 2020

AndydeCleyre force-pushed the bugfix/881 branch 3 times, most recently from 8184f16 to 638ceab Compare February 10, 2020 20:06

atugushev reviewed Feb 11, 2020

View reviewed changes

tests/test_cli_compile.py Outdated Show resolved Hide resolved

atugushev changed the title ~~Annotate primary requirements~~ Annotate primary requirements and VCS dependencies Feb 11, 2020

atugushev reviewed Feb 12, 2020

View reviewed changes

tests/test_cli_compile.py Outdated Show resolved Hide resolved

AndydeCleyre mentioned this pull request Feb 12, 2020

Choose an approach for coherent path handling #1067

Open

atugushev approved these changes Feb 16, 2020

View reviewed changes

piptools/scripts/compile.py Outdated Show resolved Hide resolved

AndydeCleyre force-pushed the bugfix/881 branch from 2658bdd to e7b2db0 Compare February 18, 2020 22:59

AndydeCleyre force-pushed the bugfix/881 branch from e7b2db0 to ca69a92 Compare February 19, 2020 18:42

atugushev merged commit 3673c3c into jazzband:master Feb 20, 2020

atugushev added this to the 4.5.0 milestone Feb 20, 2020

richafrank mentioned this pull request Feb 20, 2020

Updated README example outputs for primary req annotations #1072

Merged

2 tasks

jmbowman mentioned this pull request Feb 21, 2020

Python Requirements Update openedx/edx-platform#23156

Closed

adamchainz mentioned this pull request Feb 25, 2020

Remove "(line X)" annotations from output #1074

Closed

adamchainz mentioned this pull request Feb 26, 2020

Warn about duplicated constraints #1077

Open

karlb mentioned this pull request Mar 13, 2020

Update pip-tools raiden-network/raiden#5987

Merged

palango pushed a commit to raiden-network/raiden that referenced this pull request Mar 16, 2020

Compile requirements after pip-tools update

17a5b68

The diff is so big due to jazzband/pip-tools#1058.

hackaugusto pushed a commit to raiden-network/raiden that referenced this pull request May 29, 2020

Compile requirements after pip-tools update

4663d0e

The diff is so big due to jazzband/pip-tools#1058.

Annotate primary requirements and VCS dependencies #1058

Annotate primary requirements and VCS dependencies #1058

Conversation

AndydeCleyre commented Feb 7, 2020 • edited by atugushev Loading

Contributor checklist

codecov bot commented Feb 7, 2020 • edited Loading

Codecov Report

graingert commented Feb 7, 2020

atugushev left a comment

Choose a reason for hiding this comment

atugushev commented Feb 8, 2020 • edited Loading

atugushev commented Feb 8, 2020 • edited Loading

atugushev commented Feb 11, 2020 • edited Loading

atugushev commented Feb 11, 2020 • edited Loading

AndydeCleyre commented Feb 11, 2020

AndydeCleyre commented Feb 11, 2020

atugushev commented Feb 11, 2020

graingert commented Feb 11, 2020

AndydeCleyre commented Feb 11, 2020 • edited Loading

graingert commented Feb 11, 2020 • edited Loading

AndydeCleyre commented Feb 11, 2020

graingert commented Feb 11, 2020

AndydeCleyre commented Feb 11, 2020

graingert commented Feb 11, 2020 • edited Loading

AndydeCleyre commented Feb 11, 2020

graingert commented Feb 11, 2020 via email

atugushev commented Feb 12, 2020

atugushev left a comment

Choose a reason for hiding this comment

graingert commented Feb 18, 2020

AndydeCleyre commented Feb 18, 2020

atugushev commented Feb 19, 2020

AndydeCleyre commented Feb 19, 2020

atugushev commented Feb 19, 2020

atugushev commented Feb 20, 2020

gsakkis commented Feb 21, 2020

jmbowman commented Feb 21, 2020

AndydeCleyre commented Feb 21, 2020

graingert commented Feb 21, 2020

graingert commented Feb 21, 2020

AndydeCleyre commented Feb 21, 2020

jmbowman commented Feb 21, 2020

timmc-edx commented Feb 25, 2020

AndydeCleyre commented Feb 7, 2020 •

edited by atugushev

Loading

codecov bot commented Feb 7, 2020 •

edited

Loading

atugushev commented Feb 8, 2020 •

edited

Loading

atugushev commented Feb 8, 2020 •

edited

Loading

atugushev commented Feb 11, 2020 •

edited

Loading

atugushev commented Feb 11, 2020 •

edited

Loading

AndydeCleyre commented Feb 11, 2020 •

edited

Loading

graingert commented Feb 11, 2020 •

edited

Loading

graingert commented Feb 11, 2020 •

edited

Loading