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

ci: Optimize CI configuration and improve test setup #56

Merged
merged 4 commits into from
Jun 22, 2023

Conversation

huitseeker
Copy link
Contributor

@huitseeker huitseeker commented Jun 20, 2023

What?

Uses nextest (https://nexte.st/) and rust-cache (https://github.com/Swatinem/rust-cache) to improve CI.

  • Add new .config/nextest.toml config
  • reduce log clutter with environment variable adjustments
  • Increase CI efficiency by canceling in-progress jobs

Test & perf

Nextest shows a -33% reduction in test runtime (~6 -> ~4 min).

Successful run on my fork:
https://github.com/huitseeker/halo2curves/actions/runs/5324471954/jobs/9643816689

Notes

I remark that the build target, apart for not using release mode, is redundant with the test job (and I checked debug assertions are not used in the code base). Could we eliminate it?

Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

Thanks so much for this work! We have pending housekeeping with this repo and that helps so much!

Will enable the merging queue too and add main branch protection rules.
Also, as for the build yes, we can remove it! So feel free to. And IMO, we should add a test with no features enabled. Since now we only know results for asm and bn-table features. But we don't know anything for no-default-features.

@CPerezz
Copy link
Member

CPerezz commented Jun 21, 2023

@huitseeker
Could you please resolve the conflicts?
Also, I just updated to allow the merging queue to function as well as added more protection rules in general. I think now it should be fine.

Could you also include in the PR the merge_group: tag in the workflows so that the merging queue can be used?

Uses nextest (https://nexte.st/) and rust-cache (https://github.com/Swatinem/rust-cache) to improve CI.

- Add new `.config/nextest.toml` config
- reduce log clutter with environment variable adjustments
- Increase CI efficiency by canceling in-progress jobs
- Update Rust version to 1.63.0 in Cargo.toml
- Update CI workflow with new trigger to support merge group
- Integrate new build matrices:

Test has: { no-defauit-features, default }
Bench has: { default, asm, bn-table }
@huitseeker
Copy link
Contributor Author

huitseeker commented Jun 21, 2023

@CPerezz I have set up the merge_group triger, and the following feature matrices:
for test : { no-default-features , default },
for bench: { default, asm, bn256-table },

I'm of course happy to adjust.

I have also deleted the build job (beware, this may require changes to the branch protection settings to no longer mark this job as required from a CI PoV).

Successful run on my fork:
https://github.com/huitseeker/halo2curves/actions/runs/5336343743

@huitseeker huitseeker marked this pull request as ready for review June 21, 2023 16:12
Copy link
Contributor

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

Thanks for improving the CI so much, cargo nextest looks so great!

@han0110 han0110 enabled auto-merge June 22, 2023 03:38
@huitseeker
Copy link
Contributor Author

@CPerezz It may have been too early to turn on the merge queue: CI jobs will only fire if their trigger is on a workflow file contained in the main branch of the repo. And the merge group statements I’m adding in this PR are, well, not yet there.

IOW this PR may need manual merging, after which all others could use the merge queue.

@han0110 han0110 disabled auto-merge June 22, 2023 12:49
@CPerezz CPerezz merged commit f68b8ea into privacy-scaling-explorations:main Jun 22, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants