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

Update testing.yml #46

Merged
merged 4 commits into from
Jul 12, 2024
Merged

Update testing.yml #46

merged 4 commits into from
Jul 12, 2024

Conversation

DSilva27
Copy link
Collaborator

Caching the dependencies is breaking tests as the code in the repo does not get updated. I also removed clutter in the step that runs the tests

Caching the dependencies is breaking tests as the code in the repo does not get updated. I also removed clutter in the step that runs the tests
@DSilva27 DSilva27 requested a review from geoffwoollard July 12, 2024 18:50
@geoffwoollard
Copy link
Collaborator

There are two caches. One for dependencies (caches the environment), and one for data (caches the pulled data).

The both have keys, and get remade when the key is new. The key is hashed from, e.g. the contents of some files.

So for instance when the pyproject.toml changes, the dependency key changes, and there is a new install.

Or when the downloading script from osf changes, the files are downloaded again.

I think it's worth trying to get this working, otherwise the tests can take quite a bit longer.

@DSilva27
Copy link
Collaborator Author

DSilva27 commented Jul 12, 2024

I just know that it has happened twice that I had to remove all the cache for my tests to pass. The pyproject is not really a good way to define these keys, as the code can change without the pyproject changing. Also, the cache is taking more space than what we are supposed to use (17GB when the max is 10GB), you can see this in Actions -> caches.

@DSilva27
Copy link
Collaborator Author

Found something that is supposed to do the dependency caching for you https://github.com/actions/setup-python#caching-packages-dependencies

@geoffwoollard
Copy link
Collaborator

geoffwoollard commented Jul 12, 2024

I think caching the external pip dependencies, with the .toml file as a key hash makes sense. There is no other place that new external dependencies enter.

The tests can uninstall and install the actual code of the project (fast) with
pip uninstall cryo-em-heterogeneity-challenge-1

@geoffwoollard
Copy link
Collaborator

geoffwoollard commented Jul 12, 2024

Found something that is supposed to do the dependency caching for you https://github.com/actions/setup-python#caching-packages-dependencies

I'm fine introducing a requirements.txt file for pip.

I think it does not make any sense to have the source code mixed in with the cache - that's was not the intention for how we are using the dependency cache, and doesn't make sense to me.

@DSilva27
Copy link
Collaborator Author

DSilva27 commented Jul 12, 2024

It does not make sense, because it includes the repo itself as a dependency. It is using old versions of the code instead of the version you are pushing. That is why my test was failing, it was using the version from "main". This has two very dangerous consequences:

  1. You think your tests are passing, but they are not really passing because the version you pushed is not being used.
  2. You think your tests are not passing, but in reality they don't pass because the code is old. I found this bug because I changed the config files (added new keys), so when it validated the config file, it was using an old validator.

@geoffwoollard
Copy link
Collaborator

  • You think your tests are passing, but they are not really passing because the version you pushed is not being used.

I agree that we don't want to cache the source.

But are you sure that when you install, it uses main? It would use whatever was in the cache, if it was using a cache. But if it reinstalls it, it should be from whatever branch the PR is from. Right???

@DSilva27
Copy link
Collaborator Author

DSilva27 commented Jul 12, 2024

It uses whatever was cached. If you ran with "branch1234" and no cache exists, then the cache is created with whatever is in that branch. In this case, the cache was from main (or dev). Right now it must be from my branch iss14, not in this branch because I removed the cache. However, I bet that if you try to run a test with whatever is right now in main, the test would fail. Specifically, the svd test would fail. Maybe even the preprocessing test.

@geoffwoollard
Copy link
Collaborator

Ok let's turn off the dependency caching immediately (this PR), and look into caching just the pip requests. It will add about 2-3 min on each check. The testing can be done locally for faster turn around.

@geoffwoollard geoffwoollard self-requested a review July 12, 2024 20:42
@geoffwoollard
Copy link
Collaborator

@DSilva27 I approved. You can merge in if it's all good to go. This should be merged into main immediately after it's merged into dev.

@DSilva27
Copy link
Collaborator Author

I will merge, but I will not delete the branch just in case.

@DSilva27 DSilva27 merged commit e423526 into dev Jul 12, 2024
4 checks passed
DSilva27 added a commit that referenced this pull request Aug 6, 2024
@DSilva27 DSilva27 deleted the remove-chache-actions branch August 15, 2024 19:29
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