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.

Sign up for GitHub

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

Jump to bottom

pipenv cache is skipped when it should not be #1398

Closed
niespodd opened this issue Jan 30, 2023 · 3 comments · Fixed by #1526
Closed

pipenv cache is skipped when it should not be #1398

niespodd opened this issue Jan 30, 2023 · 3 comments · Fixed by #1526

Comments

@niespodd
Copy link

niespodd commented Jan 30, 2023 •
edited
Loading

This will silently make the buildpack skip cache when a project relies on or uses anything that relies on package(s) having git in its name:

heroku-buildpack-python/bin/steps/pipenv

Line 13 in 0519871

if ! grep -q 'git' Pipfile.lock; then

A good example is:

While I understand why the buildpack is checking for git, this should be handled differently:

  • force skip install should be behind an environment flag, given most of the users will not depend on packages installed from git+://
  • this check should look for "ref": or "git": string(s)
  • or, given lock references a particular commit hash, this check should not exist at all.

But most importantly this buildpack should notify the user why install was enforced.
@edmorley
Copy link
Member

edmorley commented Feb 8, 2023

Hi! Thank you for filing an issue - I agree the current pipenv caching handling here is broken.

So thinking about this a bit, I'm leaning towards treating lockfiles as deterministic (since well, that's the whole point of them), and not having any forced-invalidation behaviour in the buildpack at all for this. It would then be up to Pipenv to ensure it puts deterministic things in the lockfile (and not eg a branch name without a corresponding SHA).

Pipenv might very well do that already - however, I don't use Pipenv, and I haven't had a chance to play around with this. And even if Pipenv doesn't lock to a SHA in the lockfile, well that kind of seems like a bug that's up to Pipenv to fix - and so something that we shouldn't work around in the buildpack here.

Thoughts?

@edmorley
Copy link
Member

Though it seems there were issues for editable mode, xref:
#1130 (comment)

As such, it's this would need further research before we can change this.

This was referenced Jan 4, 2024
Closed
Merged
edmorley added a commit that referenced this issue Jan 5, 2024
@edmorley
Never skip running pipenv install (#1526)
a97924c 
Previously the buildpack would skip running `pipenv install` for repeat
Pipenv builds if (a) the SHA256 of the `Pipfile.lock` file had not changed
since the last successful build, and (b) there were no Git VCS references
in the lockfile.

However, this has a few issues:
1. There are other cases where it's not safe to assume that there is no
    need to re-run `pipenv install`, such as when installing a non-editable
    local dependency (see #1525), or when using editable local dependencies
    with the current path re-writing strategy (see #1520).
2. The current Git VCS check has false positives (see #1130, #1398).
3. Even if we try and add more checks/workarounds to resolve (1) and (2),
    we're still having to make assumptions about internal Pipenv implementation
    details, and hardcode these in the buildpack, hoping we didn't miss anything
    and that Pipenv's behaviour doesn't change over time (which is not the case,
    as seen by the recent regression pypa/pipenv#6054)

As such, we now instead always re-run `pipenv install`, and defer to Pipenv to
decide whether the environment needs updating.

This should still be fast, since the cached `site-packages` is still being used (and
if there are any scenarios in which it's not fast, then that's an upstream Pipenv
bug).

Integration tests were also added for various types of editable Pipenv installs,
since we previously only had test coverage of editable installs for Pip.

Fixes #1520.
Fixes #1525.
Closes #1130.
Closes #1398.
@edmorley
Copy link
Member

edmorley commented Jan 5, 2024

The fragile git check has been removed as of #1526. The pipenv install step is now run every time, however, this doesn't mean the cache isn't used. The site-packages directory is still cached - it will just now be up to Pipenv to determine whether the environment needs updating, or whether the install step can no-op.

@edmorley edmorley closed this as not planned Won't fix, can't repro, duplicate, stale Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Never skip running pipenv install heroku/heroku-buildpack-python
2 participants
@edmorley @niespodd