-
Notifications
You must be signed in to change notification settings - Fork 287
chore(ci): use aws ec2 as runner provider for cargo builds #2999
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
base: main
Are you sure you want to change the base?
Conversation
|
@soonum can we have at least a test run with the placeholder workflow ? Don't want to merge something that might break all CI |
6102d43 to
6c6befa
Compare
|
Here's a successfull run: https://github.com/zama-ai/tfhe-rs/actions/runs/19339733752?pr=2999 |
IceTDrinker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That PR is a thing of beauty, you can move the placeholder content to the right file and we'll allow it to merge
@IceTDrinker reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @soonum)
.github/workflows/placeholder_workflow.yml line 66 at r1 (raw file):
- name: Start remote instance id: start-remote-instance if: env.SECRETS_AVAILABLE == 'true'
is it supposed to always start a runner for all the possible jobs ? feels like we start instances no matter the input that would be selected for the workflow call ?
I prefer to be sure
.github/workflows/placeholder_workflow.yml line 97 at r1 (raw file):
split_runners = "${{ inputs.runners-to-use }}".replace(" ", "").split(",") runners.extend(split_runners)
formatting to be checked, some whitespaces when you copy it back to the original workflow file
.github/workflows/cargo_build.yml line 81 at r1 (raw file):
# GitHub macos-latest are now M1 macs, so use ours, we limit what runs so it will be fast # even with a few PRs runners-to-use: macos-latest-xlarge,large_windows_16_latest
maybe for clarity : extra-runners-to-use (to know that the basic CPU one is always on), but that's a nit pick if that's ok like this to you we can keep as is
ci/slab.toml line 16 at r1 (raw file):
region = "eu-west-3" image_id = "ami-0eda00173fe323828" instance_type = "m6i.16xlarge"
4xlarge not good enough ?
6c6befa to
2ea1ca5
Compare
soonum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @IceTDrinker)
.github/workflows/cargo_build.yml line 81 at r1 (raw file):
Previously, IceTDrinker wrote…
maybe for clarity : extra-runners-to-use (to know that the basic CPU one is always on), but that's a nit pick if that's ok like this to you we can keep as is
Good idea, I'll rename.
ci/slab.toml line 16 at r1 (raw file):
Previously, IceTDrinker wrote…
4xlarge not good enough ?
I've to test it to know how much it slows down our execution time.
.github/workflows/placeholder_workflow.yml line 66 at r1 (raw file):
Previously, IceTDrinker wrote…
is it supposed to always start a runner for all the possible jobs ? feels like we start instances no matter the input that would be selected for the workflow call ?
I prefer to be sure
Hmm in practice yeah, an EC2 instance would always be spawned.
That being said, since no input is required, I should add a condition at job level to check if at least one run is selected.
Something likeif: inputs.run-pcc-cpu-batch || inputs.run-pcc-hpu || ...
|
Previously, soonum (David Testé) wrote…
or you could error out before if no option is selected, this way you don't have to verify it later on |
2ea1ca5 to
2d5037a
Compare
2d5037a to
bd3adac
Compare
|
one thing to consider it to make sure the cargo build global check does not consider the stop instance result (if we have an issue with the instance provider) to avoid having a wrongly red cargo build bpr |
|
the continue on error should be enough it seems |
IceTDrinker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me as they say ! :)
thanks, only thing may be to see if we have teardown failures if we can manage to still get a green build status given the status should not rely on the teardown failing
@IceTDrinker reviewed 1 of 3 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @soonum)

Since
cargo_build_common.ymlis a new file, I'm afraid we will have to wait for this PR to be merged before being able to to test the new setup.This change is