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.

Sign up for GitHub

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

Jump to bottom

chore(zk): add bench zk v1 vs v2 #1917

Merged
merged 1 commit into from
Jan 10, 2025
Merged

chore(zk): add bench zk v1 vs v2 #1917

merged 1 commit into from
Jan 10, 2025

Conversation

nsarlin-zama
Copy link
Contributor

PR content/description

Adds benchmark of zkv1 vs zkv2 inside the tfhe crate

@cla-bot cla-bot bot added the cla-signed label Jan 7, 2025
Base automatically changed from ns/chore/params_zkv1 to main January 7, 2025 11:02
@nsarlin-zama nsarlin-zama force-pushed the ns/chore/bench_zkv1_v2 branch 2 times, most recently from d03f292 to b58bbfa Compare January 8, 2025 09:09
@nsarlin-zama nsarlin-zama changed the base branch from main to ns/fix/bench_wasm_parsing January 8, 2025 09:13
@nsarlin-zama nsarlin-zama force-pushed the ns/fix/bench_wasm_parsing branch from 8f3ed22 to cc6e468 Compare January 8, 2025 09:41
@nsarlin-zama nsarlin-zama force-pushed the ns/chore/bench_zkv1_v2 branch from b58bbfa to b753fd4 Compare January 8, 2025 10:06
Base automatically changed from ns/fix/bench_wasm_parsing to main January 8, 2025 12:23
@nsarlin-zama nsarlin-zama force-pushed the ns/chore/bench_zkv1_v2 branch 2 times, most recently from c8dfc58 to 3fec3f3 Compare January 8, 2025 13:54
@nsarlin-zama nsarlin-zama force-pushed the ns/chore/bench_zkv1_v2 branch from 3fec3f3 to 2353e85 Compare January 8, 2025 14:48
soonum
soonum approved these changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@soonum soonum left a comment

Choose a reason for hiding this comment

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

👍

@IceTDrinker
Copy link
Member

is the bench parser fixed though ?

@nsarlin-zama
Copy link
Contributor Author

yes, we had 2 issues:

  • one with versioned names which has been fixed here: fix(bench): use correct name for parameters in wasm benches #1929
  • second issue in this pr because I prefixed compute parameters with ZKV1, but compute parameters are identical in V1 and V2. So I fixed it by appending ZKV1 or ZKV2 in the bench name and instead of the parameters name

@nsarlin-zama
Copy link
Contributor Author

manual run here: https://github.com/zama-ai/tfhe-rs/actions/runs/12672979600

@IceTDrinker
Copy link
Member

manual run here: https://github.com/zama-ai/tfhe-rs/actions/runs/12672979600

perfect, did you check that the graphana display was good ?

@nsarlin-zama
Copy link
Contributor Author

I'm on it

@nsarlin-zama nsarlin-zama merged commit 4c8d55f into main Jan 10, 2025
113 checks passed
@nsarlin-zama nsarlin-zama deleted the ns/chore/bench_zkv1_v2 branch January 10, 2025 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IceTDrinker IceTDrinker IceTDrinker approved these changes

@soonum soonum soonum approved these changes

Assignees
No one assigned
Labels
approved cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nsarlin-zama @IceTDrinker @soonum @zama-bot