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

Add lock feature to make it easier to test if you need to run pip-compile #1070

Closed
wants to merge 6 commits into from

Conversation

lfdebrux
Copy link

@lfdebrux lfdebrux commented Feb 18, 2020

Our team @alphagov is currently migrating our repositories away from a custom pip freeze script to using pip-tools.

One thing that we've missed though is the ability to quickly check if we need to run pip-compile (either as a GitHub check, or a pre-commit script). Currently the only way to do this that we were able to see is running pip-compile and seeing if the output file has changed. However, this can take a while, and also isn't idempotent.

This pull request adds a "lock" feature that makes it easier to see if the files that were used to generate a requirements file have changed. The workflow we'd like to see is:

  1. generate a "locked" requirements file using pip-compile --lock requirements.in
  2. test it with pip-check requirements.txt!

If requirements.in has changed since pip-compile was run, pip-check should print a message telling you to run pip-compile again, and exit with a non-zero status so that automated tools can pick this up.

In the suggested implementation, this is done by adding a checksum for each input file (including transitively included input files) to the output file; pip-check then reads the checksum and tests whether the input files still match the checksum. This is simple enough that it could be done in a shell script, but having it integrated into pip-tools makes for a nicer experience I feel!

As an added bonus, checking is also done when running pip-compile --lock, so that if the checksums are still valid then pip-compile doesn't need to do any dependency resolution.

Changelog-friendly one-liner: Add lock feature to make it easier to test if you need to run pip-compile

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 Feb 18, 2020

Codecov Report

Merging #1070 into master will increase coverage by 0.01%.
The diff coverage is 99.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1070      +/-   ##
==========================================
+ Coverage   99.38%   99.40%   +0.01%     
==========================================
  Files          34       38       +4     
  Lines        2450     2865     +415     
  Branches      301      334      +33     
==========================================
+ Hits         2435     2848     +413     
- Misses          8        9       +1     
- Partials        7        8       +1     
Impacted Files Coverage Δ
tests/test_lock.py 98.63% <98.63%> (ø)
piptools/__main__.py 100.00% <100.00%> (ø)
piptools/lock.py 100.00% <100.00%> (ø)
piptools/scripts/check.py 100.00% <100.00%> (ø)
piptools/scripts/compile.py 100.00% <100.00%> (ø)
piptools/writer.py 99.32% <100.00%> (+0.06%) ⬆️
tests/test_cli_check.py 100.00% <100.00%> (ø)
tests/test_cli_compile.py 100.00% <100.00%> (ø)
tests/test_writer.py 100.00% <100.00%> (ø)
... and 3 more

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 175ed22...487e4b6. Read the comment docs.

@lfdebrux lfdebrux force-pushed the ldeb-add-lock branch 11 times, most recently from 1bd8e75 to 47ec35c Compare February 19, 2020 09:09
@atugushev
Copy link
Member

Hello @lfdebrux,

Thanks for your contribution! That feature looks interesting. I'll try it locally and review it carefully soon.

P.S.
In the future, I'd prefer you'd create an issue with the "feature request" label first, where we could discuss the value of it, a user interface and implementation details. You wrote a good readable code fully covered by tests, that's awesome! I highly appreciate your time, thus I'd be disappointed with your wasted energy if it turns out that PR would not be merged. Thanks for understanding 🙏

@atugushev atugushev added the enhancement Improvements to functionality label Feb 21, 2020
@lfdebrux
Copy link
Author

Hi @atugushev ,

Thank you for your message, and thank you so much for offering to review it.

I totally understand what you're saying; with hindsight that seems really obvious and I feel a bit silly for not creating an issue first. Although I do think this would be a useful feature, it's completely fine if it doesn't end up getting merged in the form as I've suggested. Please feel free to make any suggestions for changes or whatever!

Thanks again for taking a look, I appreciate it :)

@lfdebrux
Copy link
Author

Hi @atugushev, I appreciate it's a weird time for everyone right now, but I was wondering if you had had a chance to look at this PR?

If it would help I would be happy to create a feature request issue.

@atugushev
Copy link
Member

atugushev commented Mar 29, 2020

@lfdebrux

Thanks for pinging me. I've tried it locally and it works! Though, It has a bug with files included via -c option (see workflow for layered requirements), but generally, it looks cool.


Ping also @vphilippon @codingjoe @AndydeCleyre and @jazzband/pip-tools. What do you think folks? Any feedback is appreciated!

@AndydeCleyre
Copy link
Contributor

I like the general idea! But I have some additional concerns.

  • The nothing-to-do check exits when the input file has not changed, but other options determine a different output. Well, I guess it is appropriate given the description of the feature, but I found it unintuitive.

After lock-compiling with a header, trying to do so again without a header (to get rid of it) will not work, as the lock-check exits with "nothing to do." Same deal trying to go from no-header to header.

Other output-changing options that don't rely on differing input files include --allow-unsafe, --generate-hashes, --no-annotate, and options that affect dep resolution like --pre, --index-url, --extra-index-url, --find-links, etc. Also env vars like CUSTOM_COMPILE_COMMAND.

Even outside the options, it's possible that repository package releases would get revoked (for security reasons), and the resolution ought to change. Maybe the answer to that one is to avoid using the --lock option if trouble shows up, I don't know.

But again, the flag description does clearly state: "If output file already exists and has a valid lock, resolving dependencies is skipped." So maybe that's all fine.

  • I would prefer a different term than "lock," as similar tools generally use that to mean something more like what this project terms "compile." I haven't thought of better language yet. Maybe with a different term the above concern would matter less, if it made the behavior more obvious -- something like pip-compile --if-input-changed (but less verbose, and also indicated that it created the hash...). Another way may be to split this into two flags, with one that implies the other, like --hash-input which doesn't abort based on hash equality, and --if-input-changed or --on-input-change or something, which implies --hash-input.

  • It seems reasonable that this feature doesn't support stdin, but really it wouldn't be too hard to that either. People may like to feed files in that way and we can still hash the input.

  • Maybe there should be a more minimal comment if --no-header is passed (just the single line)

Anyway, it seems like a good idea.

@vphilippon
Copy link
Member

I have the same thoughts/reflexion that @AndydeCleyre listed. Still pondering one some of them in my head at this point.

It feels weird to me to check the need to run pip-compile based on if the requirements.in has changed. In my wokflow(s), I tend to run pip-compile quite often to pick up updates from new releases and continuously tests those.
I'd be curious to know a bit more on the use case, in order to better understand how pip-tools usage differs from the usage I have in my head, and why someone (say, @lfdebrux 😄 ) would want to only pip-compile on requirements.in changes rather than pick up new updates on a regular basis.

As @atugushev pointed out, thanks for the great contribution @lfdebrux !

@lfdebrux
Copy link
Author

Thanks for all the comments!

I'd be curious to know a bit more on the use case, in order to better understand how pip-tools usage differs from the usage I have in my head, and why someone (say, @lfdebrux 😄 ) would want to only pip-compile on requirements.in changes rather than pick up new updates on a regular basis.

@vphilippon I will try and describe the situation that made us need something like this, but I hope the feature will be good for other use cases.

The particular use case we have is that we use an automated tool (https://pyup.io) for keeping our dependencies up to date; outside of the PRs raised by that tool we don't usually update our requirements files. Unfortunately pyup doesn't run pip-compile for you, so we have it currently set to just update requirements.in, and then we have to remember to run pip-compile ourselves before merging the PR. We wanted a script that would make our tests fail if pyup had updated the requirements.in file but the requirements.txt file hadn't been updated; I wrote something in bash but someone suggested it might be better to have it in Python.

The nothing-to-do check exits when the input file has not changed, but other options determine a different output. Well, I guess it is appropriate given the description of the feature, but I found it unintuitive.

I see your point @AndydeCleyre, especially as you pointed out there are lots of flags that will change the output but not the input. This feature is not strictly necessary given pip-check, as you could just run pip-check && pip-compile --lock (or whatever instead of --lock).

I would prefer a different term than "lock"...

Yeah I'm not sure about "lock" either. If the nothing-to-do check were removed from pip-compile then it could be something like --hash or --checksum and not have to worry about the --if-input-changed flag.

It seems reasonable that this feature doesn't support stdin, but really it wouldn't be too hard to that either

Agreed, I just didn't get around to it 😁

Maybe there should be a more minimal comment if --no-header is passed (just the single line)

👍

@lfdebrux
Copy link
Author

It has a bug with files included via -c option (see workflow for layered requirements)

@atugushev what is the bug? I thought I included a test for that 🤔

@webknjaz
Copy link
Member

Unfortunately pyup doesn't run pip-compile for you, so we have it currently set to just update requirements.in, and then we have to remember to run pip-compile ourselves before merging the PR.

FYI there's another tool (https://dependabot.com/) belonging to GitHub nowadays. It does run pip-compile automatically and automerges its PRs if you configure it t do so.

@lfdebrux
Copy link
Author

Unfortunately pyup doesn't run pip-compile for you, so we have it currently set to just update requirements.in, and then we have to remember to run pip-compile ourselves before merging the PR.

FYI there's another tool (https://dependabot.com/) belonging to GitHub nowadays. It does run pip-compile automatically and automerges its PRs if you configure it t do so.

Yeah we will probably end up using that, it's not as nice as PyUp in some ways though.

@sfdye
Copy link
Member

sfdye commented Mar 31, 2020

Yeah we will probably end up using that, it's not as nice as PyUp in some ways though.

@lfdebrux Just curious, in what ways?

@lfdebrux
Copy link
Author

Yeah we will probably end up using that, it's not as nice as PyUp in some ways though.

@lfdebrux Just curious, in what ways?

The main thing dependabot is missing for us is grouping together updates into one pull request. I'm aware that it's something they're working on, it's issue #5.

@webknjaz
Copy link
Member

@lfdebrux TBH pyup also has problems distributing the load: pyupio/pyup#331.

@lfdebrux
Copy link
Author

lfdebrux commented Apr 3, 2020

Maybe there should be a more minimal comment if --no-header is passed (just the single line)

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants