Skip to content

Conversation

@AvivYossef-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/run_and_compare_benchmark branch from c85fcbc to d47a32f Compare October 23, 2025 09:36
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_committer_benchmark_ci branch 2 times, most recently from 1176af8 to aed808e Compare October 23, 2025 10:08
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/run_and_compare_benchmark branch from d47a32f to 54cdcf1 Compare October 23, 2025 10:08
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_committer_benchmark_ci branch from aed808e to 6331f24 Compare October 23, 2025 11:31
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/run_and_compare_benchmark branch 2 times, most recently from 26f8f7c to d21ba87 Compare October 23, 2025 12:12
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_committer_benchmark_ci branch from 6331f24 to 68338c6 Compare October 23, 2025 12:12
@avi-starkware
Copy link
Collaborator

.github/workflows/committer_ci.yml line 134 at r1 (raw file):

      - run: bash ./crates/starknet_committer_and_os_cli/benches/bench_split_and_prepare_post.sh benchmarks_list.txt benchmark_comment_body.txt

      - run: echo BENCHES_RESULT=$(cat benchmark_comment_body.txt) >> $GITHUB_ENV

After your change, you no longer post a comment on github.
Is that intentional?

Code quote:

      # Benchmark the new code, splitting the benchmarks, and prepare the results for posting a comment.
      - run: bash ./crates/starknet_committer_and_os_cli/benches/bench_split_and_prepare_post.sh benchmarks_list.txt benchmark_comment_body.txt

      - run: echo BENCHES_RESULT=$(cat benchmark_comment_body.txt) >> $GITHUB_ENV

Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @avi-starkware)


.github/workflows/committer_ci.yml line 134 at r1 (raw file):

Previously, avi-starkware (Avi Cohen) wrote…

After your change, you no longer post a comment on github.
Is that intentional?

Yes, I don't find it useful.
I added informative prints to the command instead

Copy link
Collaborator

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

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

Do you plan to keep this in the committer_ci ?
What about the gateway benchmark you added in the config?

@avi-starkware reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, all discussions resolved

Copy link
Collaborator

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

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

@avi-starkware reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)

@avi-starkware
Copy link
Collaborator

.github/workflows/committer_ci.yml line 111 at r1 (raw file):

      # Benchmark the current branch and compare to the previous run.
      # (downloads inputs from GCS, fails if regression > 8%).
      - run: cargo run -p bench_tools -- run-and-compare --package starknet_committer_and_os_cli --out /tmp/new_results --regression-limit 8.0

Can we cache the inputs?

Code quote:

      # (downloads inputs from GCS, fails if regression > 8%).
      - run: cargo run -p bench_tools -- run-and-compare --package starknet_committer_and_os_cli --out /tmp/new_results --regression-limit 8.0

Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-starkware left a comment

Choose a reason for hiding this comment

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

In the committer-ci, we will run it with
--package starknet_committer_and_os_cli
No one asked me to add the gateway benchmark to the CI.
I have added it to the benchmark config before I had access to the committer input files,
to verify that it works

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware)


.github/workflows/committer_ci.yml line 111 at r1 (raw file):

Previously, avi-starkware (Avi Cohen) wrote…

Can we cache the inputs?

Yes, we can.
The CLI tool doesn't support it right now.
But I can add bash commands for it.

Copy link
Collaborator

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)


.github/workflows/committer_ci.yml line 111 at r1 (raw file):

Previously, AvivYossef-starkware wrote…

Yes, we can.
The CLI tool doesn't support it right now.
But I can add bash commands for it.

Try to use the inputs from cache at least in this second command. No need to download the files twice, and you already have the command to read inputs from a local dir.

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_committer_benchmark_ci branch from 68338c6 to e8c1565 Compare October 28, 2025 06:31
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/run_and_compare_benchmark branch from 168fbdc to bf4411b Compare October 28, 2025 08:21
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_committer_benchmark_ci branch 2 times, most recently from 7987461 to f75ee3c Compare October 28, 2025 11:13
@AvivYossef-starkware AvivYossef-starkware marked this pull request as ready for review October 28, 2025 11:19
Copy link
Contributor Author

Done

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_committer_benchmark_ci branch from f75ee3c to bc2c8da Compare October 29, 2025 07:18
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/fix_committer_benchmark_config branch 2 times, most recently from 2914c8c to 0067392 Compare October 29, 2025 07:25
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_committer_benchmark_ci branch 2 times, most recently from 7803264 to 44bc61f Compare October 29, 2025 07:38
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/fix_committer_benchmark_config branch from 0067392 to 7c93885 Compare October 29, 2025 07:38
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_committer_benchmark_ci branch from 44bc61f to befbbc9 Compare October 29, 2025 07:40
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/fix_committer_benchmark_config branch from 7c93885 to 1dd17d0 Compare October 29, 2025 07:40
Copy link
Collaborator

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

@avi-starkware reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_committer_benchmark_ci branch from befbbc9 to 4efa19e Compare October 30, 2025 09:23
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/fix_committer_benchmark_config branch from 1dd17d0 to 9d3ea9a Compare October 30, 2025 09:23
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/fix_committer_benchmark_config branch from 9d3ea9a to ec0bfbc Compare October 30, 2025 10:50
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_committer_benchmark_ci branch from 4efa19e to cb58e04 Compare October 30, 2025 10:50
@AvivYossef-starkware AvivYossef-starkware changed the base branch from aviv/fix_committer_benchmark_config to main-v0.14.1 October 30, 2025 12:30
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_committer_benchmark_ci branch from cb58e04 to 83c943d Compare October 30, 2025 12:41
Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-starkware left a comment

Choose a reason for hiding this comment

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

@AvivYossef-starkware reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)

@AvivYossef-starkware AvivYossef-starkware added this pull request to the merge queue Oct 30, 2025
Merged via the queue into main-v0.14.1 with commit cf045b9 Oct 30, 2025
20 of 24 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants