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

tests(ci): use BuildJet for ubuntu jobs #13943

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

tests(ci): use BuildJet for ubuntu jobs #13943

wants to merge 9 commits into from

Conversation

connorjclark
Copy link
Collaborator

Default github action runner for ubunutu has 7gb ram and 2 cpu cores. see https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources

These BuildJet runners have 4cpu and 16gb ram

I picked this tier because it matches the price GitHub gives for their machines (although I don't know if our GoogleChrome org even pays that, I think public repos may be free? not sure).

Need to test more, but first look says maybe 2-4 minutes faster?

with buildjet https://github.com/GoogleChrome/lighthouse/actions/runs/2241966717
with github https://github.com/GoogleChrome/lighthouse/actions/runs/2241937334

Also could be we have far less problems with provisioning (github queuing our jobs)

@connorjclark connorjclark requested a review from a team as a code owner April 28, 2022 21:38
@connorjclark connorjclark requested review from brendankenny and removed request for a team April 28, 2022 21:38
@brendankenny
Copy link
Member

I picked this tier because it matches the price GitHub gives for their machines (although I don't know if our GoogleChrome org even pays that, I think public repos may be free? not sure).

we fall under the enterprise plan

@@ -59,7 +59,7 @@
"watch": "yarn unit-core --watch",
"unit:cicoverage": "yarn c8 --all yarn unit:ci",
"coverage": "yarn unit:cicoverage && c8 report --reporter html",
"coverage:smoke": "yarn c8 yarn smoke -j=1 && c8 report --reporter html",
"coverage:smoke": "yarn c8 yarn smoke && c8 report --reporter html",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uh, we don't even use this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uh, we don't even use this.

maybe it's for running locally?

@@ -53,7 +53,7 @@ jobs:
- run: sudo apt-get install xvfb
- name: Run smoke tests
run: |
xvfb-run --auto-servernum yarn c8 yarn smoke --debug -j=1 --retries=2 --shard=${{ matrix.smoke-test-shard }}/$SHARD_TOTAL
xvfb-run --auto-servernum yarn c8 yarn smoke --debug --retries=2 --shard=${{ matrix.smoke-test-shard }}/$SHARD_TOTAL
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have as much cpu contention now that we went from 2->4 cpus, so let's try this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe set it to 4?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(thanks to @adamshiervani for the idea!)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to help! 🚀

Copy link
Member

@brendankenny brendankenny Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have as much cpu contention now that we went from 2->4 cpus, so let's try this

no one ever updated it for GHA either, that's a holdover from travis. It should be fine, because all the perf sensitive ones are set to run serially. The default is 5 jobs at once, I have no idea why. Maybe that was the number of manual batches we used before that refactor. Maybe it's the number that runs ok on a 4 core desktop machine 🤷

package.json Outdated
@@ -37,7 +37,7 @@
"start": "yarn build-report --standalone && node ./lighthouse-cli/index.js",
"jest": "node --experimental-vm-modules ./node_modules/jest/bin/jest.js",
"test": "yarn diff:sample-json && yarn lint --quiet && yarn unit && yarn type-check",
"test-bundle": "yarn smoke --runner bundle -j=1 --retries=2",
"test-bundle": "yarn smoke --runner bundle --retries=2",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, doesn't seem to like it. Maybe you can try 4, like you previously suggested!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the same thing locally, so I guess we have to run these tests serially. others seemed fine though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-j=1 was added at inception of the smokehouse bin: https://github.com/GoogleChrome/lighthouse/pull/9943/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R27

the failures are because we are clearing marky in every run of lighthouse, which is fine but the bundled smokehouse is the only one that actually runs lighthouse in the same process everytime

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-j=1 was added at inception of the smokehouse bin: https://github.com/GoogleChrome/lighthouse/pull/9943/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R27

that's the same PR?

the failures are because we are clearing marky in every run of lighthouse, which is fine but the bundled smokehouse is the only one that actually runs lighthouse in the same process everytime

Ah, I didn't look at the logs. That'll be two places it fails then :) I assume that's from the FR changes. Just say no to singletons

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hah, I misread and thought that linked to your other comment above....

Copy link
Collaborator Author

@connorjclark connorjclark Apr 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could change lighthouse-runners/bundle.js to use a new process for each run (make a new file bundle-entry.js that just takes url, configJson, fraggleRock from argv) then we could remove this restriction. wdyt @brendankenny ? This runner is the odd one out, all the others make a new process for each run.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's kind of annoying in that we should be able to use performance marks (and debug) in a way that doesn't run afoul of sharing a process, but you're right this is the odd one out, so it's not a huge deal.

  • this might work with node workers? Maybe no real benefit but it could be a single file solution (run the test unless isMainThread, in which case run itself as a worker)
  • if we're really lazy, sharding works with the bundle runner as well, so can always shard these (and the existing smoke shard count can definitely be reduced when they run so fast on these runners :)

@brendankenny
Copy link
Member

Should we land this before #13968? I assume we have all the approvals we need?

@@ -53,7 +53,7 @@ jobs:
- run: sudo apt-get install xvfb
- name: Run smoke tests
run: |
xvfb-run --auto-servernum yarn c8 yarn smoke --debug -j=1 --retries=2 --shard=${{ matrix.smoke-test-shard }}/$SHARD_TOTAL
xvfb-run --auto-servernum yarn c8 yarn smoke --debug --retries=2 --shard=${{ matrix.smoke-test-shard }}/$SHARD_TOTAL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, does coverage work without -j=1? c8 is supposed to sum over child_processes but smokehouse might be too tricky for it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we even trust the coverage report these days?

the linked coverage: https://codecov.io/gh/GoogleChrome/lighthouse/compare/f2db965f75a4f9d2a84614a3cd5a1466a41e99bc...ea372c7da03ac84e4b398df80123c962be008d34/changes

is saying that all the FR stuff is unused. but we have non-smoke tests that cover that. so...

@connorjclark
Copy link
Collaborator Author

Should we land this before #13968?

I was thinking we should do #13968 first so we can get a clearer before/after of timing differences from changing to BuildJet.

I assume we have all the approvals we need?

Yup!

@brendankenny
Copy link
Member

I was thinking we should do #13968 first so we can get a clearer before/after of timing differences from changing to BuildJet.

cool, though for a completely fair comparison, some of the current -j=1 should maybe be bumped to -j=2 :)

@crohr
Copy link

crohr commented Jan 23, 2024

I suppose this is no longer needed now that GitHub doubled the vCPU and RAM count of default runners. If you'd like to experiment with much bigger CPU counts, RunsOn might also be an option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants