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

Improve cache Poetry install instructions #154

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

sebastian-correa
Copy link
Contributor

In #150, I described a usecase were caching the Poetry installation was causing the dependency cache step to break when the .lock was updated.

In summary, if some action used a cached poetry install (skips running snok/install-poetry), the configurations set in with would not be done, which caused any new dependency installs to not be cached, as Poetry by default installs venvs not in the .venv path (so the cache step can't find the .venv directory to cache).

This PR adds a section in the docs to clarify this and guide users to a fix.

README.md Outdated
Comment on lines 512 to 542
Note that when the cache hits and the *Install Poetry* step is skipped, all configurations done during install are skipped. Therefore, the loaded Poetry will run with default settings.

This may break the cache of Python dependencies when the `.lock` file changes and the Poetry *install* is recovered from an old cache. For example, if you set

```yaml
with:
virtualenvs-in-project: true
```

and have a cache step for `.venv`, the `install-poetry` will run and configure Poetry to create the venv in `.venv` *the first time it is run*. Dependencies will install and will be cached. However, when you update your `.lock` file, the Poetry installation will be recovered from cache, and a new venv will be created in the default Poetry location (not `.venv`, because the install poetry step doesn't cache the settings), so the cache venv step will fail (as there's no `.venv` directory to cache).

To remedy this, you should use this instead

```yaml
name: test

on: pull_request

jobs:
test:
runs-on: ubuntu-latest
steps:
- name: Check out repository
uses: actions/checkout@v4
- name: Set up python
uses: actions/setup-python@v5
with:
python-version: '3.12'
- name: Load cached Poetry installation
id: cached-poetry
uses: actions/cache@v3
with:
path: ~/.local # the path depends on the OS
key: poetry-0 # increment to reset cache
- name: Install Poetry
if: steps.cached-poetry.outputs.cache-hit != 'true'
uses: snok/install-poetry@v1
with:
virtualenvs-create: true
virtualenvs-in-project: true
- name: Configure poetry
if: steps.cached-poetry.outputs.cache-hit == 'true'
run: poetry config virtualenvs.in-project true
```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this as a starting point, but think it needs to be shortened a bit. What do you think of this?


Note that when the cache is hit, and the Install Poetry step is skipped, configuration options are not re-applied. The cached Poetry installation will now run with default settings. To make things work the same, you may add a dedicated configuration step for this case:

- name: Configure poetry
  if: steps.cached-poetry.outputs.cache-hit == 'true'
  run: poetry config virtualenvs.in-project true

Or consider using a config.toml file to store you configuration options. See details in the Poetry configuration docs.

Copy link
Contributor Author

@sebastian-correa sebastian-correa Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed. Verbosity is my weakness, so thanks for your input, which I think is much better.

Edit: also rebased with main.

@sebastian-correa sebastian-correa force-pushed the improve-caching-instructions branch from 0a8dade to f5314b1 Compare July 23, 2024 19:06
@sondrelg
Copy link
Member

Thanks for the help here @sebastian-correa! 🙇

@sondrelg sondrelg merged commit 929c2d5 into snok:main Jul 23, 2024
66 of 67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants