Skip to content

fix(ci): update committer benchmark comment #6360

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

Merged
merged 1 commit into from
May 11, 2025

Conversation

aner-starkware
Copy link
Contributor

No description provided.

@aner-starkware aner-starkware self-assigned this May 7, 2025
@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

github-actions bot commented May 7, 2025

fs.readFileSync('bench_new.txt', 'utf8'),

@aner-starkware aner-starkware force-pushed the aner/fix_committer_comment branch 3 times, most recently from fa38c6d to c7d304c Compare May 7, 2025 12:07
Copy link

github-actions bot commented May 7, 2025

$(fs.readFileSync('bench_new.txt', 'utf8')),

@aner-starkware aner-starkware force-pushed the aner/fix_committer_comment branch from c7d304c to 79929fa Compare May 7, 2025 12:26
Copy link

github-actions bot commented May 7, 2025

Benchmark movements: No major performance changes detected.

@aner-starkware aner-starkware force-pushed the aner/fix_committer_comment branch 8 times, most recently from 88e6e52 to 15c63ad Compare May 7, 2025 15:08
@aner-starkware aner-starkware changed the title wip fix(committer): update committer benchmark comment May 7, 2025
@aner-starkware aner-starkware force-pushed the aner/fix_committer_comment branch 2 times, most recently from 20bdc79 to efacce8 Compare May 8, 2025 05:47
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)


a discussion (no related file):
this looks like a bug, can you open another PR above this one to test this?
image.png

@aner-starkware
Copy link
Contributor Author

Previously, dorimedini-starkware wrote…

this looks like a bug, can you open another PR above this one to test this?
image.png

It is not a bug; it was a bug that's already fixed (this text no longer appears in the PR).

Copy link
Collaborator

@dorimedini-starkware dorimedini-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 @aner-starkware and @nimrod-starkware)


a discussion (no related file):

Previously, aner-starkware wrote…

It is not a bug; it was a bug that's already fixed (this text no longer appears in the PR).

ok, but I don't see the committer report, only the No major performance changes detected message.
can you open a dummy PR above this one that just triggers the committer benchmark?
trigger it, see what the first message is, amend the commit without no changes, push again to re-trigger, and make sure all is well

@aner-starkware
Copy link
Contributor Author

Previously, dorimedini-starkware wrote…

ok, but I don't see the committer report, only the No major performance changes detected message.
can you open a dummy PR above this one that just triggers the committer benchmark?
trigger it, see what the first message is, amend the commit without no changes, push again to re-trigger, and make sure all is well

I already saw the committer report - it looks almost the same, except now there are no newlines (the format changed). How do I "trigger" the committer report? I can just push again in the commit, but unless there are any changes, it will print this message.

Copy link
Collaborator

@dorimedini-starkware dorimedini-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 @aner-starkware and @nimrod-starkware)


a discussion (no related file):

Previously, aner-starkware wrote…

I already saw the committer report - it looks almost the same, except now there are no newlines (the format changed). How do I "trigger" the committer report? I can just push again in the commit, but unless there are any changes, it will print this message.

that's why you need a new dummy PR above this one, not a new commit

@aner-starkware
Copy link
Contributor Author

Previously, dorimedini-starkware wrote…

that's why you need a new dummy PR above this one, not a new commit

What's the difference? I don't follow. I can do it without a new commit even, it will still trigger the benchmarking and update the comment. Only problem is that the updated comment could possibly be equivalent to the existing one... Previously there was a comment with modifications, but it was already overwritten.

@aner-starkware aner-starkware force-pushed the aner/fix_committer_comment branch from efacce8 to f37aad3 Compare May 8, 2025 12:22
Copy link
Collaborator

@dorimedini-starkware dorimedini-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 (commit messages unreviewed), 1 unresolved discussion (waiting on @aner-starkware and @nimrod-starkware)


a discussion (no related file):

Previously, aner-starkware wrote…

What's the difference? I don't follow. I can do it without a new commit even, it will still trigger the benchmarking and update the comment. Only problem is that the updated comment could possibly be equivalent to the existing one... Previously there was a comment with modifications, but it was already overwritten.

if your feature works, then pushing new commits to this PR won't every create a new comment, because there is an existing one.
I want to test the effect on a proper benchmarking comment, hence, new PR

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @nimrod-starkware)

@aner-starkware aner-starkware changed the title fix(committer): update committer benchmark comment fix(ci): update committer benchmark comment May 8, 2025
@aner-starkware
Copy link
Contributor Author

Previously, dorimedini-starkware wrote…

if your feature works, then pushing new commits to this PR won't every create a new comment, because there is an existing one.
I want to test the effect on a proper benchmarking comment, hence, new PR

עדיין לא הצלחתי להבין מה בדיוק אתה רוצה לראות, אבל הנה
https://reviewable.io/reviews/starkware-libs/sequencer/6389

@aner-starkware
Copy link
Contributor Author

Previously, aner-starkware wrote…

עדיין לא הצלחתי להבין מה בדיוק אתה רוצה לראות, אבל הנה
https://reviewable.io/reviews/starkware-libs/sequencer/6389

אגב, למרות שהוספתי שם הדפסות, לא נראה שינוי (ופה דווקא כן - אולי יש הטייה לטובה לשינוי...)

Copy link
Collaborator

@dorimedini-starkware dorimedini-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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)


a discussion (no related file):

Previously, aner-starkware wrote…

אגב, למרות שהוספתי שם הדפסות, לא נראה שינוי (ופה דווקא כן - אולי יש הטייה לטובה לשינוי...)

hmmm... ok so your feature replaces previous text with new text each time, right?
now i understand why this conversation was a broken phone lol
:lgtm:

@aner-starkware aner-starkware added this pull request to the merge queue May 11, 2025
Merged via the queue into main with commit 21d6947 May 11, 2025
22 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.

3 participants