-
-
Notifications
You must be signed in to change notification settings - Fork 856
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 Python venv cache #1680
👷 Add Python venv cache #1680
Conversation
Pipeline failing due to |
@Kludex This is pretty sweet. The time difference when looking at individual jobs is even better:
That's a 4x improvement. :-) Also a more sober approach that's nicer with PyPI and network usage. I would be happy to take this in, I'll let @tomchristie see if that's "not too fiddly" enough to be worth using across Encode? |
id: cache-requirements | ||
with: | ||
path: ${{ env.pythonLocation }} | ||
key: ${{ env.pythonLocation }}-${{ matrix.python-version }}-${{ hashFiles('requirements.txt', 'setup.py') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where have you gotten ${{ env.pythonLocation }}
from?
(I mean, it looks neat, but I don't understand it)
I don't see anything about it mentioned in the docs, here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took it from this tutorial.
But it's defined on the setup-action
above. Here specifically: https://github.com/actions/setup-python/blob/dc73133d4da04e56a135ae2246682783cc7c7cb6/src/find-python.ts#L62
Should we make sure that we have more strictly pinned dependancies before pulling this in? Eg... https://github.com/encode/httpcore/pull/184/files |
I'm always in favor of pinning development dependencies. Do you want to have the same setup on all |
@Kludex We experimented with Dependabot for HTTPCore. To be honest I'm not entirely convinced we need it, over a regular "bump dependencies" PR once in a while. On HTTPCore Dependabot bumps deps every month, even only for patch releases; those PR need to be merged one at a time, with rebasing between each… I'm not sure we need it. I'd say we can start with pinning deps, get this in, then decide if we want Dependabot? |
What if we do what is suggested in this article for HTTPCore: https://www.hrvey.com/blog/combine-dependabot-prs I'll pin the dependencies here in the meantime. |
So, I wouldn't include the pinning as part of this. |
I assumed that your question was a requirement for this 😅 We can do that or just invalidate the cache in n days? |
Possibly. Not absolutely necessary - even with pinning we won't end up with a fully 100% absolutely guaranteed identical installs between runs, because of various edge cases. Even if we do want to treat it as a requirement, it'd still make sense to PR it separately, since I think that pinning our deps is definitely something we'd like, whereas this pull request is probably something that we'd like. (Having spent a long time maintaining projects, I'm almost always on the side of keeping PRs well isolated wherever possible.) |
Is there another case than dependencies of dependencies that you're including here? |
Reverted requirements. |
Ha, not really - I think I was a bit tired, and couldn't think about describing "transitive dependancies". It's possible? there's also some edges around eg. deps varying on diff environments, meaning we can't hard-pin some cases? Dunno. |
@florimondmanca I've tested this GitHub action on this repository. It has a minimal setup, only for demonstration purposes, and it worked very well (please check the closed PRs to understand the timeline). @tomchristie what do you think about adding this bot? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This PR aims to add a cache to the Python environment.
I also took the audacious move to indent with an empty line between each action, so let me know if you wish for it to be removed.
Discussion on Gitter
@Kludex:
@tomchristie:
Reference: https://gitter.im/encode/community?at=60c3235cc705e53c1c86be7e
Docs: https://github.com/actions/cache