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

Pipenv install is skipped in cases where it's not safe to do so #1525

Closed
edmorley opened this issue Jan 4, 2024 · 0 comments · Fixed by #1526
Closed

Pipenv install is skipped in cases where it's not safe to do so #1525

edmorley opened this issue Jan 4, 2024 · 0 comments · Fixed by #1526
Assignees
Labels

Comments

@edmorley
Copy link
Member

edmorley commented Jan 4, 2024

Currently the buildpack skips re-running pipenv install for repeat builds iff (a) the SHA256 of the Pipfile.lock file has not changed since the last successful build, and (b) the Pipfile.lock file does not contain git references:

if [[ -f Pipfile.lock ]]; then
if [[ -f .heroku/python/Pipfile.lock.sha256 ]]; then
if [[ $(openssl dgst -sha256 Pipfile.lock) == $(cat .heroku/python/Pipfile.lock.sha256) ]]; then
# Don't skip installation if there are git deps.
if ! grep -q 'git' Pipfile.lock; then
echo "Skipping installation, as Pipfile.lock hasn't changed since last deploy." | indent
mcount "tool.pipenv"
export SKIP_PIPENV_INSTALL=1
export SKIP_PIP_INSTALL=1
fi
fi
fi
fi

However, there are other cases where pipenv install still needs to be run.

For example, imagine a Pipfile containing:

[packages]
mypackage = {file = "packages/mypackage", editable = false}

Since editable = false the local package will be copied into site-packages on the initial build (rather than the original source linked via a .pth file, as happens for editable installs). And then on subsequent builds if no other changes have been made to Pipfile.lock, the pipenv install step will be skipped, even though changes could have been made to files under packages/mypackage/.

(Note: The above is assuming a Pipenv version is being used that is not affected by the regression pypa/pipenv#6054, since otherwise the install will be editable regardless of editable = false)

Whilst we could add "editable" as another property to check within the Pipfile.lock file:

Ultimately, any optimisation strategy here requires us to make assumptions about pipenv implementation details, and hardcode some of those details in the buildpack, which isn't ideal.

As such, I think we should instead always run pipenv install, and defer to pipenv itself to make the determination as to whether any changes are required. Pipenv install should not take too long to run if there are no changes required (and if it does, that's an upstream bug that pipenv should fix, and not something we should hack around and risk environment correctness issues).

GUS-W-14762837.

@edmorley edmorley self-assigned this Jan 5, 2024
@edmorley edmorley added the bug label Jan 5, 2024
edmorley added a commit that referenced this issue Jan 5, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant