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

[Feature] Speed up the CI tests #65

Open
willGraham01 opened this issue Jan 3, 2024 · 8 comments
Open

[Feature] Speed up the CI tests #65

willGraham01 opened this issue Jan 3, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@willGraham01
Copy link
Collaborator

Not sure how this fits into the future plans for this package, but our CI currently has a runtime of over 2 hours. When we're using our internal runner this presumably won't be a problem, requiring mandatory status checks that take 2+hours on each PR to succeed doesn't seem healthy for our development speed (particular if there's an error in one of the middle/late tests!).

Wondering if it's possible to only run a subset of tests (maybe just the user-facing package tests) on CI so that we know we're shipping something that works. Then we can leave the heavy-lifting checks to our internal machines that can cope with the load?

@willGraham01 willGraham01 added the enhancement New feature or request label Jan 3, 2024
@adamltyson
Copy link
Member

100%, the tests need to be much faster, otherwise development of brainmapper will be a real chore. @sfmig what takes so long? Presumably the tests run on CI shouldn't need to take any longer than the original brainreg/cellfinder tests?

@willGraham01
Copy link
Collaborator Author

My suspicion is that it's trying to run all of our benchmarks (meant for internal runners) in addition to the tests that came with the package, but can investigate properly when Sofia's back from leave (next week) so I don't break anything.

So if we direct the CI to the correct subfolders of tests we might get around this issue?

@adamltyson
Copy link
Member

The original idea is that there will be tests on small datasets for CI and large datasets on internal runners. @alessandrofelder may know what's going on, but as you say, we can wait until next week.

@alessandrofelder
Copy link
Member

I don't think it's running benchmarks.

I think the issue is that the workflow tests are currently slow - there is a plan to speed them up by caching and mocking. See #34 and #35. In retrospect, we maybe should have solved this issue before merging the PR... but we wanted to keep some momentum going!

@adamltyson
Copy link
Member

@alessandrofelder why are they so slow, compared to say the cellfinder & brainreg tests? Are they just using data that's too big?

@alessandrofelder
Copy link
Member

alessandrofelder commented Jan 4, 2024

I think it's (at least partly) because they download the cellfinder test data from GIN via pooch three times.

@sfmig
Copy link
Contributor

sfmig commented Jan 10, 2024

Sorry this has taken a bit longer than expected due to sick leave + Christmas break, but should now be improved in this PR. Happy to discuss if we want further improvements!

@sfmig
Copy link
Contributor

sfmig commented Jan 11, 2024

Suggested next steps:

  • further reduce testing time in CI using Github's cache action,
  • mock Path.home() only on local repos, and not on CI, so that CI tests are more realistic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants