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

MG CI is too slow #1028

Open
GitGab19 opened this issue Jul 3, 2024 · 13 comments · Fixed by #1056
Open

MG CI is too slow #1028

GitGab19 opened this issue Jul 3, 2024 · 13 comments · Fixed by #1056
Assignees
Labels

Comments

@GitGab19
Copy link
Collaborator

GitGab19 commented Jul 3, 2024

As we discussed during last dev call, the current MG CI execution is too slow. We need to improve it, by using caches, multi-threading, or docker containers to run things in parallel.
I just found an interesting article regarding this stuff: https://www.uffizzi.com/blog/optimizing-rust-builds-for-faster-github-actions-pipelines

@pavlenex
Copy link
Collaborator

pavlenex commented Jul 3, 2024

Perhaps slightly related to #928 but maybe not, certainly connected so cross-linking them to help us track similar isuses.

@plebhash
Copy link
Collaborator

plebhash commented Jul 5, 2024

one potential mitigation strategy that @Fi3 pointed out during the call is to restructure coverage.yaml

currently, all MG tests are lumped together into the same job: message-generator-test

as we get false alarms, our only recourse is to click this button on the PR Actions UI while hoping the next run won't have any false alarms and we can finally merge the PR:
image

this strategy forces all MG tests to re-run (because they're all lumped together into the same job), as opposed to only the false alarms... this is suboptimal for multiple reasons:

  • it gives room for new false alarms on tests that were already successfully executed in the previous run
  • it makes the process much slower, because we need to wait for all MG tests again, as opposed to only the ones that had a false alarm

if we could break the different MG tests into separate jobs under coverage.yaml, this process would be a bit better

of course, this is not the ultimate solution for the issue here, it is only a mitigation strategy while we don't find a way to bring deterministic execution to the MG tests under Github CI

@plebhash
Copy link
Collaborator

plebhash commented Jul 5, 2024

to expand a bit on my previous comment, I see two potential courses of action for this issue:

  • a paliative solution, which makes MG CI slightly less worse by breaking tests into multiple jobs and allowing us to re-run false alarms in a more agile way
  • a definitive solution, which makes MG CI execution deterministic

@plebhash plebhash added the good first issue Good for newcomers label Jul 9, 2024
@johnnyasantoss
Copy link
Contributor

I want to work on this

@pavlenex pavlenex mentioned this issue Jul 9, 2024
@pavlenex pavlenex added this to the 1.0.2 milestone Jul 9, 2024
@johnnyasantoss
Copy link
Contributor

Related discussion: #944

@jbesraa
Copy link
Contributor

jbesraa commented Jul 10, 2024

just a note: i think it would be better to speedup the GH action check initially if its a quick win before working on improving the local setup

@Fi3
Copy link
Collaborator

Fi3 commented Jul 10, 2024

just a note: i think it would be better to speedup the GH action check initially if its a quick win before working on improving the local setup

100% agree

@johnnyasantoss
Copy link
Contributor

I'm splitting the work in multiple steps. The first part is to add cache to the build and avoid recompiling unchanged deps and running tests in parallel. The other steps we will still need to discuss it later on.

@pavlenex pavlenex linked a pull request Jul 16, 2024 that will close this issue
plebhash added a commit to plebhash/stratum that referenced this issue Jul 16, 2024
this is a paliative solution to stratum-mining#1028

we make MG CI slightly less worse by breaking tests into multiple jobs and allowing us to re-run false alarms in a more agile way

MG Tests are also separated from Tarpaulin
GitGab19 pushed a commit that referenced this issue Jul 17, 2024
this is a paliative solution to #1028

we make MG CI slightly less worse by breaking tests into multiple jobs and allowing us to re-run false alarms in a more agile way

MG Tests are also separated from Tarpaulin
@GitGab19
Copy link
Collaborator Author

Partially solved by #1056

@plebhash
Copy link
Collaborator

Partially solved by #1056

actually this ended up being #1056 + #1060 + #1064

now we are able to re-run MG CI jobs independently, which helps bypass false alarms

however, optimizations on MG CI are still desirable, so @johnnyasantoss feel free to continue working on this if you want

@pavlenex
Copy link
Collaborator

@GitGab19 @plebhash Is there anything concrete that @johnnyasantoss should work on? Since we had #1056 #1060 #1064 my question is, is there enough context on what needs to be done, or should we specify that for @johnnyasantoss? I guess a question for @johnnyasantoss as well 😄

@GitGab19
Copy link
Collaborator Author

I think that we could just try to optimize what done by @plebhash. For example adding cache where possible, since it's also a thing that @johnnyasantoss mentioned here: #1028 (comment)

@johnnyasantoss
Copy link
Contributor

@pavlenex I have enough context on this and can get this done. Just need to figure out a better time organization.

@GitGab19 Yep. I have a local branch with optimizations on the CI side. I'll push the changes soon

@pavlenex pavlenex modified the milestones: 1.0.2, 1.0.3 Aug 11, 2024
@plebhash plebhash removed this from the 1.0.3 milestone Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress 🏗️
Development

Successfully merging a pull request may close this issue.

6 participants