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

Add tests of pypi installation in nightly workflow #357

Merged
merged 7 commits into from
Jan 23, 2024
Merged

Add tests of pypi installation in nightly workflow #357

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5918be8
Try testing pypi installation in nightly
pvandyken Dec 18, 2023
931d395
Add .github to sparse checkout
pvandyken Dec 18, 2023
66d8d31
Add containers to sparse checkout
pvandyken Dec 18, 2023
3f26ef0
Install directly from repo
pvandyken Dec 18, 2023
c2d2a45
Update cache-key
pvandyken Dec 18, 2023
f0eac06
Fix permissions and dependency versions
pvandyken Jan 22, 2024
80e93ad
Fix formatting
pvandyken Jan 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 58 additions & 16 deletions .github/workflows/nightly.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,11 @@ on:
- cron: "5 3 * * 2"

jobs:
build-cache-env:
runs-on: ubuntu-latest
strategy:
matrix:
python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"]
steps:
- uses: khanlab/actions/.github/actions/[email protected]
id: setup
with:
python-version: ${{ matrix.python-version }}
- name: setup docker
uses: ./.github/actions/build-test-container
with:
python-version: ${{ steps.setup.outputs.python-version }}

calibrate-timings:
runs-on: ubuntu-latest
needs: ["build-cache-env"]
permissions:
pull-requests: write
contents: write

steps:
- name: install
Expand Down Expand Up @@ -76,3 +61,60 @@ jobs:
body: |
The number of tests has changed since the last generated test-timings
file. This PR contains an automatically regenerated file.

build-cache-env:
runs-on: ubuntu-latest
strategy:
matrix:
python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"]
steps:
- uses: khanlab/actions/.github/actions/[email protected]
id: setup
with:
python-version: ${{ matrix.python-version }}
cache-key: pypi
install-deps-only: dev
install-library: false

- name: build docker test container
uses: ./.github/actions/build-test-container
with:
python-version: ${{ steps.setup.outputs.python-version }}
load: true

- name: install lib
Copy link
Contributor

Choose a reason for hiding this comment

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

More out of curiousity, is there a reason for installing the library after building the docker test container / is this used at all downstream? (I assume the docker test container is making use of that dev-only environment somehow).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is to cache everything installed by pip. I don't think it matters what order the docker and installation are done

Copy link
Contributor

@kaitj kaitj Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, I'll see if I can dig into this a little bit more this week - from a brief comparison between the test part of the workflow and the the build-cache-env, I'm not sure I am seeing where things are getting cached (for the repeated installation steps).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything installed by pip gets cached. So the test step inherits the pip cache and will install libs from their where applicable. But not sure if I'm understanding the confusion

Copy link
Contributor

@kaitj kaitj Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ahh, realized I was actually comparing the wrong thing (the workflow checks vs the changes to the nightly workflow). Nonetheless, I am not sure I seeing how building the docker container and installing the lib is caching for the downstream steps.

In the workflow checks at the moment (without having dug too deep into the logs):

  1. The parts of each step using installPyProject is making use of the cache.
  2. The docker container gets built, where it restores from a relatively small cache before going through all of the steps of building the container (this is repeated for each test split), where it appears to be downloading the libraries. This is repeated from the build-env-cache to the test splits (again downloading the libraries).
  3. I don't actually see where the install lib is happening in the logs, but I think I need to dig deeper here (I think I am just missing where it switches from building the docker to installing with pip here, so some of the comments from 2 might be more relevant here for 3).

This might also be a case of workflow checks not using the most up-to-date workflow changes.

Copy link
Contributor

@kaitj kaitj Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed that the docker cache is a separate issue, though the pip install (the one where it is being called with install lib), doesn't appear to be cached either (from what I can make out in the logs). Note that I am basing this off of the workflows that are being run with similar steps).

We can address these in a separate issue / PR given that this is does add the testing of the PyPI installation and that part at least seems to be working. If it doesn't cache in the nightly we can revisit this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

though the pip install (the one where it is being called with install lib), doesn't appear to be cached either (from what I can make out in the logs).

This is what I'm confused about, I'm not running that workflow. (or are you looking at the current weekly run? But I don't think that uses the install lib thing).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh you can ignore this altogether - I was looking at the wrong workflow. I was looking at the current test workflows which doesn't use install lib. Will keep an eye on it in the nightly run once that gets going.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at some historic runs, and I do think you were in fact correct. It doesn't look like the cache is being used. But I think this is probably more a shortcoming of the khanlab actions installer... it might not be caching the correct directory.

Docker caching is annoying, but not something I feel super motivated to fix 😒

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's something we can keep an eye on and fix if needed / eventually.

As far as Docker caching goes, I wouldn't worry about it too much - it doesn't look like it takes too long to build the container. I'll leave this link here as a reference if we do end up trying to look into docker caching:

https://dev.to/dtinth/caching-docker-builds-in-github-actions-which-approach-is-the-fastest-a-research-18ei

run: poetry run pip install .

test:
runs-on: ubuntu-latest
needs: [ 'build-cache-env' ]
strategy:
matrix:
python-version: ['3.8', '3.9', '3.10', '3.11', '3.12']
split: ['1', '2', '3', '4', '5']
fail-fast: false
steps:
- uses: khanlab/actions/.github/actions/[email protected]
id: setup
with:
python-version: ${{ matrix.python-version }}
cache-key: nightly
install-deps-only: dev
install-library: false

- name: install lib
pvandyken marked this conversation as resolved.
Show resolved Hide resolved
run: poetry run pip install .

- name: build docker test container
uses: ./.github/actions/build-test-container
with:
python-version: ${{ steps.setup.outputs.python-version }}
load: true

- name: Test with pytest
env:
HYPOTHESIS_PROFILE: pr
run: >-
poetry run pytest -n auto --splits 5 --group ${{ matrix.split }}
--doctest-modules --ignore=docs
--ignore=snakebids/project_template --benchmark-disable