MDAnalysis / UserGuide

User Guide for MDAnalysis
https://userguide.mdanalysis.org
22 stars 31 forks source link

ci: enable micromamba env cache #273

Closed jandom closed 1 year ago

jandom commented 1 year ago

The micromamba environment creation takes about 5 minutes and is more than the actual build step (2 mins for buliding docs). Here is an example run

https://github.com/MDAnalysis/UserGuide/actions/runs/5622842883/job/15236245154

This is not ideal – developers want to see a crisp output of their work.

I'm not sure how to enable this feature, or how well it works, so using this branch to experiment. If it works well, maybe we can consider porting to MDA core?

Update Here are some preliminary results showing the impact on the build speed, with and without caching

Workflow Name Caching on/off Setup Duration Job Duration
mda_gh_ci Off 4m 39s 6m 16s
mda_gh_ci On 21s 2m 6s
mda_gh_ci_tests Off 6m 29s 16m 39s
mda_gh_ci_tests On 27s 15m 26s
IAlibay commented 1 year ago

maybe we can consider porting to MDA core?

We can't unfortunately. I already looked into this some time ago. Because of the size of the matrix the amount of data stored becomes excessive. Our build times are tiny so it's not a big deal tbh.

jandom commented 1 year ago

That makes sense – yeah I can see how the build matrix can inflate the artifact size – but it's still a nice improvement to this repo?

IAlibay commented 1 year ago

That makes sense – yeah I can see how the build matrix can inflate the artifact size – but it's still a nice improvement to this repo?

I think so, but I'm trying to properly look at the impact, I might need to punt a complete look to the weekend.

  1. There's a branch, develop precendence here that we need to consider. So the improvements only exist if CI ran on develop earlier in the day, or if you ran CI in the PR earlier in the same day.
  2. There's a cache clash I'm going to quickly write a review comment about now.
  3. We can't change the cache time limit or enforce an override. So it looks as if your PR suddenly requires an upstream fix, you'll need to wait a day until the cache invalidates?
IAlibay commented 1 year ago

There's a branch, develop precendence here that we need to consider. So the improvements only exist if CI ran on develop earlier in the day, or if you ran CI in the PR earlier in the same day.

From initial thoughts, I don't really care about this too much. At worse you get no improvement, at best it helps. It doesn't look like we'd get a regression so that's fine.

We can't change the cache time limit or enforce an override. So it looks as if your PR suddenly requires an upstream fix, you'll need to wait a day until the cache invalidates?

This I care about a lot more. A lot of the userguide is "emergency patch fixes". I am concerned it might slow us down in those cases (unless I'm missing something that allows us to invalidate the cache easily).

jandom commented 1 year ago

I've addressed the cache clash, so that should be fine – good catch!

Very wise re emergency invalidation but I think we can just wipe the caches here? https://github.com/MDAnalysis/UserGuide/actions/caches Optional tho, could be just more faff than it's worth – I'm 50/50 in my head right now

IAlibay commented 1 year ago

I've addressed the cache clash, so that should be fine – good catch!

Very wise re emergency invalidation but I think we can just wipe the caches here? https://github.com/MDAnalysis/UserGuide/actions/caches Optional tho, could be just more faff than it's worth – I'm 50/50 in my head right now

Yeah (edit: deleting) cache manually might be enough. I don't really mind tbh, @MDAnalysis/coredevs does anyone have strong feelings here?

RMeli commented 1 year ago

No strong feelings, but I think that if the cache can be removed manually, then it's OK.

jandom commented 1 year ago

hi there many thanks for all the reviews and the merge (although that one was unexpected)