search

TopEFT / topeft

15 stars 24 forks source link

Conda swap space #386

Closed bryates closed 1 year ago

bryates commented 1 year ago

Increase swap space to fixe conda timing out. Same fix that was merged in TopCoffea (https://github.com/TopEFT/topcoffea/pull/11)

codecov[bot] commented 1 year ago

Codecov Report

Merging #386 (3aa26f1) into master (082e011) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #386   +/-   ##
=======================================
  Coverage   28.12%   28.12%           
=======================================
  Files          29       29           
  Lines        4849     4849           
=======================================
  Hits         1364     1364           
  Misses       3485     3485
Flag Coverage Δ
unittests 28.12% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Andrew42 commented 1 year ago

@bryates, the only thing that jumps out to me is that, if you can, you should pin the repo that's used to implement this to a specific commit (in this case it would be the most recent one probably). Otherwise, you sort of open yourself up to whatever wild changes might get pushed to a repo you don't control.

bryates commented 1 year ago

@bryates, the only thing that jumps out to me is that, if you can, you should pin the repo that's used to implement this to a specific commit (in this case it would be the most recent one probably). Otherwise, you sort of open yourself up to whatever wild changes might get pushed to a repo you don't control.

This doesn't really use another repo, unless you mean the GitHub action it calls.

klannon commented 1 year ago

This seems like a bit of a band-aid. Why is our CI using more than 7 GB of memory to run? (You probably already know this but "swap space" is hard drive space that a computer can "swap" memory into if the memory allocated to the process is exhausted. Of course, switching from memory to hard drive space comes with a huge performance hit which is almost always something that should be avoided unless there is truly no other choice. Standard Ubuntu GH runners have 7 GB memory, so if we're swapping, it means we're going over that.)

bryates commented 1 year ago

This seems like a bit of a band-aid. Why is our CI using more than 7 GB of memory to run? (You probably already know this but "swap space" is hard drive space that a computer can "swap" memory into if the memory allocated to the process is exhausted. Of course, switching from memory to hard drive space comes with a huge performance hit which is almost always something that should be avoided unless there is truly no other choice. Standard Ubuntu GH runners have 7 GB memory, so if we're swapping, it means we're going over that.)

I'm surprised too, but it seems to be tied to the conda install step. I'm not sure why this recently became an issue, but it seams like our environment is taking up a lot of space as it is downloading and unpacking everything. This isn't an issue with mamba (even though mamba downloads multiple packages at once). My environment on earth (post installation) uses 4.2 GB, which doesn't line up with my hypothesis.

kmohrman commented 1 year ago

I also do not understand why the environment creation started intermittently failing, or why it's due to memory (based on the error message and googling I was guessing some kind of timeout). But since this fix from @bryates worked and was very non-invasive, we thought it would be a good temporary fix to at least get the CI operational again (before hopefully moving everything to mamba in the future). That was the motivation for merging the same "band-aid" PR for topcoffea anyway (and I'm guessing @bryates and @Andrew42 had the same motivations here in topeft as well).

Based on the discussion at the meeting last Friday, I think we would still want to move to mamba (and in fact Brent has such a PR open for topcoffea). So I think we still plan to move in that direction (sorry @bryates for being so slow to review the mamba PR, it's on my list but I'd like to try out mamba myself first before we merge that for topcoffea and that has not happened yet.)

klannon commented 1 year ago

Thanks for the context. I had missed that this is happening in the conda step. Sounds like mamba is the right long-term fix.

bryates commented 1 year ago

Thanks for the context. I had missed that this is happening in the conda step. Sounds like mamba is the right long-term fix.

I agree. I have a PR open now in the topcoffea repo to use mamba (https://github.com/TopEFT/topcoffea/pull/9). It works fine in the CI.