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

Add tests of pypi installation in nightly workflow #357

Merged
merged 7 commits into from
Jan 23, 2024

Conversation

pvandyken
Copy link
Contributor

Rather than just run the usual test workflow with the pinned dependencies, I thought it would be more useful to test installing the project from pypi with the latest of all dependencies. So this contains the attempt to do that. The code depends on a custom branch in khanlab/actions, but if everything works here, I'll make a merge of that branch

@kaitj
Copy link
Contributor

kaitj commented Dec 18, 2023

More of a thought before you dive too far into this, wondering about the installing from pypi. How does that change if dependencies change or is there a dev version getting pushed to pypi as well?

I guess choice of installation will matter depending on what kind of testing needs to be done?

@pvandyken
Copy link
Contributor Author

More of a thought before you dive too far into this, wondering about the installing from pypi. How does that change if dependencies change or is there a dev version getting pushed to pypi as well?

So I'm changing the goal post a bit: I'm going to install the main branch, but use pip instead of poetry (just use poetry for testing deps) so that pinned versions don't get used. This should greatly simplify the workflow

@pvandyken pvandyken force-pushed the maint/ci/nightly-fix branch 5 times, most recently from 553039f to 14dcdc8 Compare December 18, 2023 20:48
@pvandyken pvandyken force-pushed the maint/ci/nightly-fix branch from f391346 to f0eac06 Compare January 22, 2024 15:57
@pvandyken pvandyken marked this pull request as ready for review January 22, 2024 15:57
@pvandyken
Copy link
Contributor Author

@kaitj, hopefully I've fixed the permission issue causing the weekly failure on the live version of this workflow

@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (19215f8) 90.26% compared to head (80e93ad) 55.67%.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #357       +/-   ##
===========================================
- Coverage   90.26%   55.67%   -34.59%     
===========================================
  Files          32       32               
  Lines        1489     1489               
===========================================
- Hits         1344      829      -515     
- Misses        145      660      +515     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@kaitj kaitj left a comment

Choose a reason for hiding this comment

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

Largely looks good to me, just a couple of nitpicky comments and a question.

.github/workflows/nightly.yml Outdated Show resolved Hide resolved
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

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

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

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

.github/workflows/nightly.yml Show resolved Hide resolved
@pvandyken pvandyken requested a review from kaitj January 22, 2024 22:10
@pvandyken pvandyken merged commit 939aac0 into khanlab:main Jan 23, 2024
34 checks passed
@pvandyken pvandyken deleted the maint/ci/nightly-fix branch January 23, 2024 17:51
@pvandyken pvandyken added the maintenance Updates or improvements that do not change functionality of the code label Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Updates or improvements that do not change functionality of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants