chore(blockifier_reexecution): add blockifier reexecution test to ci #1857

aner-starkware · 2024-11-06T12:15:23Z

No description provided.

reviewable-StarkWare · 2024-11-06T12:15:30Z

This change is

github-actions · 2024-11-06T12:15:47Z

Artifacts upload triggered. View details here

github-actions · 2024-11-06T13:48:13Z

Artifacts upload triggered. View details here

github-actions · 2024-11-18T11:58:43Z

Artifacts upload triggered. View details here

codecov · 2024-11-18T12:09:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 42.14%. Comparing base (e3165c4) to head (bf185f6).
Report is 509 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1857      +/-   ##
==========================================
+ Coverage   40.10%   42.14%   +2.04%     
==========================================
  Files          26      204     +178     
  Lines        1895    23943   +22048     
  Branches     1895    23943   +22048     
==========================================
+ Hits          760    10091    +9331     
- Misses       1100    13386   +12286     
- Partials       35      466     +431

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

github-actions · 2024-11-18T12:18:57Z

Artifacts upload triggered. View details here

github-actions · 2024-11-18T12:25:27Z

Artifacts upload triggered. View details here

github-actions · 2024-11-18T12:41:19Z

Artifacts upload triggered. View details here

github-actions · 2024-11-18T13:03:48Z

Artifacts upload triggered. View details here

github-actions · 2024-11-18T13:12:53Z

Artifacts upload triggered. View details here

github-actions · 2024-11-18T13:21:01Z

Artifacts upload triggered. View details here

github-actions · 2024-11-18T13:26:30Z

Artifacts upload triggered. View details here

github-actions · 2024-11-19T08:13:53Z

Artifacts upload triggered. View details here

github-actions · 2024-11-19T14:43:04Z

Artifacts upload triggered. View details here

aner-starkware · 2024-11-19T15:06:52Z

crates/blockifier_reexecution/src/state_reader/raw_rpc_json_test.rs line 170 at r1 (raw file):

#[case::v_0_13_0(600001)]
// Commented out until fix integrated.
// #[case::v_0_13_1(620978)]

Blocking myself until rebased and can be uncommented

Code quote:

// Commented out until fix integrated.
// #[case::v_0_13_1(620978)]

dorimedini-starkware

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)

a discussion (no related file):
we now need a README.md file for this crate. Can you write one?
(it can be a separate PR)
the readme should cover:

All CLI commands and their meaning
How to run the RPC tests (using TEST_URL=<ENDPOINT>)

crates/blockifier_reexecution/src/state_reader/raw_rpc_json_test.rs line 170 at r1 (raw file):

Previously, aner-starkware wrote…

Blocking myself until rebased and can be uncommented

also blocking because blocking self sometimes doesn't actually work

crates/blockifier_reexecution/resources/offline_reexecution_files_prefix line 1 at r1 (raw file):

4f7def520

not that it matters much, but... where is this value from?

Code quote:

4f7def520

.github/workflows/blockifier_ci.yml line 57 at r1 (raw file):

      # transaction_serde is not activated by any workspace crate; test the build.
      - run: cargo build -p blockifier --features transaction_serde
      - run: cargo test -p blockifier --features transaction_serde

I think this is still worth keeping. I know reexecution feature always activates this serde feature but we shouldn't assume this will be true forever IMO.

Code quote:

      # transaction_serde is not activated by any workspace crate; test the build.
      - run: cargo build -p blockifier --features transaction_serde
      - run: cargo test -p blockifier --features transaction_serde

.github/workflows/blockifier_reexecution_ci.yml line 9 at r1 (raw file):

      - main-v[0-9].**
    tags:
      - v[0-9].**

do we want to run this CI on post-merge? I doubt it; it should only block PRs

Code quote:

  push:
    branches:
      - main
      - main-v[0-9].**
    tags:
      - v[0-9].**

.github/workflows/blockifier_reexecution_ci.yml line 11 at r1 (raw file):

      - v[0-9].**
    # TODO(Dori, 1/9/2024): Decide when exactly native-blockifier artifacts will be built. Until
    #   then, keep the 'paths' key empty and build on every push to a release branch / tag.

irrelevant (copied from somewhere?)

Code quote:

    # TODO(Dori, 1/9/2024): Decide when exactly native-blockifier artifacts will be built. Until
    #   then, keep the 'paths' key empty and build on every push to a release branch / tag.

.github/workflows/blockifier_reexecution_ci.yml line 26 at r1 (raw file):

      - '.github/workflows/blockifier_ci.yml'
      - '.github/workflows/upload_artifacts_workflow.yml'
      - 'build_native_in_docker.sh'

why should these three trigger this workflow?

Code quote:

      - '.github/workflows/blockifier_ci.yml'
      - '.github/workflows/upload_artifacts_workflow.yml'
      - 'build_native_in_docker.sh'

.github/workflows/blockifier_reexecution_ci.yml line 31 at r1 (raw file):

      - 'crates/blockifier/**'
      - 'crates/native_blockifier/**'
      - 'scripts/build_native_blockifier.sh'

why? reexecution doesn't depend on native blockifier

Code quote:

      - 'crates/native_blockifier/**'
      - 'scripts/build_native_blockifier.sh'

.github/workflows/blockifier_reexecution_ci.yml line 44 at r1 (raw file):

  blockifier_reexecution:
    runs-on: starkware-ubuntu-latest-medium
    if: ${{ github.event_name == 'pull_request' }}

see above - don't run this workflow on push events at all, and remove this if

Code quote:

if: ${{ github.event_name == 'pull_request' }}

.github/workflows/blockifier_reexecution_ci.yml line 52 at r1 (raw file):

        uses: "google-github-actions/auth@v2"
        with:
          credentials_json: ${{ secrets.SA_REEXECUTION_ARTIFACTS_READ_ACCESS_KEY }}

the point with public access was to allow running this CI without secrets.
otherwise, external PRs from external devs cannot run the CI.
why do you need this for read access?

Code quote:

${{ secrets.SA_REEXECUTION_ARTIFACTS_READ_ACCESS_KEY }}

.github/workflows/blockifier_reexecution_ci.yml line 57 at r1 (raw file):

      - run: gcloud storage cp -r gs://reexecution_artifacts/$REEXECUTION_INPUT_FILES_PREFIX/resources/* ./crates/blockifier_reexecution/resources/
      # Run blockifier re-execution tests.
      - run: cargo test --release -p blockifier_reexecution -- --ignored

I can see that the reexecution itself did not run. the raw json tests ran, but that's it.

once you fix this, please check which is faster: with or without --release.

how much time required for compilation in both cases?
how much time required for running the test in both cases?

Code quote:

- run: cargo test --release -p blockifier_reexecution -- --ignored

dorimedini-starkware

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)

.github/workflows/blockifier_reexecution_ci.yml line 22 at r1 (raw file):

    paths:
      # Other than code-related changes, all changes related to the blockifier should trigger the build.
      - 'crates/blockifier_reexecution/**'

move this down (sort this list)

Code quote:

- 'crates/blockifier_reexecution/**'

aner-starkware

Reviewable status: 1 of 4 files reviewed, 12 unresolved discussions (waiting on @AvivYossef-starkware and @dorimedini-starkware)

.github/workflows/blockifier_reexecution_ci.yml line 9 at r1 (raw file):

Previously, dorimedini-starkware wrote…

do we want to run this CI on post-merge? I doubt it; it should only block PRs

Done.

.github/workflows/blockifier_reexecution_ci.yml line 11 at r1 (raw file):

Previously, dorimedini-starkware wrote…

irrelevant (copied from somewhere?)

Done.

.github/workflows/blockifier_reexecution_ci.yml line 22 at r1 (raw file):

Previously, dorimedini-starkware wrote…

move this down (sort this list)

Done.

.github/workflows/blockifier_reexecution_ci.yml line 26 at r1 (raw file):

Previously, dorimedini-starkware wrote…

why should these three trigger this workflow?

I basically copied everything from the blockifier CI, reasoning that if there's a reason to run the blockifier checks, then reexecution checks should also be run

.github/workflows/blockifier_reexecution_ci.yml line 44 at r1 (raw file):

Previously, dorimedini-starkware wrote…

see above - don't run this workflow on push events at all, and remove this if

Done.

.github/workflows/blockifier_reexecution_ci.yml line 52 at r1 (raw file):

Previously, dorimedini-starkware wrote…

the point with public access was to allow running this CI without secrets.
otherwise, external PRs from external devs cannot run the CI.
why do you need this for read access?

That's not what I recall; look at your request in the conversation.

.github/workflows/blockifier_reexecution_ci.yml line 57 at r1 (raw file):

Previously, dorimedini-starkware wrote…

I can see that the reexecution itself did not run. the raw json tests ran, but that's it.

once you fix this, please check which is faster: with or without --release.

how much time required for compilation in both cases?

how much time required for running the test in both cases?

OK, will check.

aner-starkware · 2024-11-19T17:02:28Z

Previously, dorimedini-starkware wrote…

we now need a README.md file for this crate. Can you write one?
(it can be a separate PR)
the readme should cover:

All CLI commands and their meaning

How to run the RPC tests (using TEST_URL=<ENDPOINT>)

OK, will do it in a separate PR.

aner-starkware

Reviewable status: 0 of 4 files reviewed, 12 unresolved discussions (waiting on @AvivYossef-starkware and @dorimedini-starkware)

crates/blockifier_reexecution/resources/offline_reexecution_files_prefix line 1 at r1 (raw file):

Previously, dorimedini-starkware wrote…

not that it matters much, but... where is this value from?

Hash of some commit; probably already amended that commit or rebased, but the hash doesn't change (since it's the folder - need to change it manually)

crates/blockifier_reexecution/src/state_reader/raw_rpc_json_test.rs line 170 at r1 (raw file):

Previously, dorimedini-starkware wrote…

also blocking because blocking self sometimes doesn't actually work

Done.

aner-starkware

Reviewable status: 0 of 4 files reviewed, 12 unresolved discussions (waiting on @AvivYossef-starkware and @dorimedini-starkware)

.github/workflows/blockifier_ci.yml line 57 at r1 (raw file):

Previously, dorimedini-starkware wrote…

I think this is still worth keeping. I know reexecution feature always activates this serde feature but we shouldn't assume this will be true forever IMO.

Done.

aner-starkware · 2024-11-19T17:22:35Z

.github/workflows/blockifier_reexecution_ci.yml line 57 at r1 (raw file):

Previously, aner-starkware wrote…

OK, will check.

Actually, I think it was reversed - the reexecution tests ran, but all other tests did not. Now all tests run... But I'll split the tests file (in a separate PR), so that it will make more sense.

dorimedini-starkware

Reviewed 2 of 3 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)

.github/workflows/blockifier_reexecution_ci.yml line 26 at r1 (raw file):

Previously, aner-starkware wrote…

I basically copied everything from the blockifier CI, reasoning that if there's a reason to run the blockifier checks, then reexecution checks should also be run

If there's a reason to run blockifier checks, then yes,
but

changes in the blockifier CI are changes in the checks, not changes in the crate, so no reason to trigger reexecution checks
native blockifier is not a dependency of reexecution, so changes in the native blockifier shouldn't trigger reexecution - the blockifier CI also runs native blockifier stuff (actually, only the upload_artifacts workflow should really be triggered on changes to native blockifier, we can probably update the blockifier CI triggers to reflect this).

dorimedini-starkware

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)

.github/workflows/blockifier_reexecution_ci.yml line 57 at r1 (raw file):

Previously, aner-starkware wrote…

Actually, I think it was reversed - the reexecution tests ran, but all other tests did not. Now all tests run... But I'll split the tests file (in a separate PR), so that it will make more sense.

without --release, compilation takes 3:20 and reexecution seems to take ~15 minutes.
also, I don't see that the 0_13_1_1 case finished, but the tests seem to have passed... why?
also, 15 minutes is a bit much, please try again with --release... hopefully it should decrease the runtime dramatically

aner-starkware · 2024-11-19T17:50:21Z

.github/workflows/blockifier_reexecution_ci.yml line 57 at r1 (raw file):

Previously, dorimedini-starkware wrote…

without --release, compilation takes 3:20 and reexecution seems to take ~15 minutes.
also, I don't see that the 0_13_1_1 case finished, but the tests seem to have passed... why?
also, 15 minutes is a bit much, please try again with --release... hopefully it should decrease the runtime dramatically

Without --release: compilation 4 minutes, tests 14 minutes (total: 18 minutes); this solution isn't scalable, if you test reexecution on more blocks then it will grow significantly...

With --release: compilation 10 minutes, tests 1 minute (total 11 minutes); this solution is more scalable since you can add many tests and it won't dramatically increase the time

dorimedini-starkware

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)

.github/workflows/blockifier_reexecution_ci.yml line 57 at r1 (raw file):

Previously, aner-starkware wrote…

Without --release: compilation 4 minutes, tests 14 minutes (total: 18 minutes); this solution isn't scalable, if you test reexecution on more blocks then it will grow significantly...

With --release: compilation 10 minutes, tests 1 minute (total 11 minutes); this solution is more scalable since you can add many tests and it won't dramatically increase the time

cool! let's go with --release then

aner-starkware · 2024-11-19T17:52:22Z

.github/workflows/blockifier_reexecution_ci.yml line 57 at r1 (raw file):

Previously, dorimedini-starkware wrote…

cool! let's go with --release then

test state_reader::raw_rpc_json_test::test_block_reexecution::case_03_v_0_13_1_1 ... ok
Seems to have finished...

dorimedini-starkware

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)

.github/workflows/blockifier_reexecution_ci.yml line 57 at r1 (raw file):

Previously, aner-starkware wrote…

test state_reader::raw_rpc_json_test::test_block_reexecution::case_03_v_0_13_1_1 ... ok
Seems to have finished...

ah, missed that..

aner-starkware

Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @AvivYossef-starkware and @dorimedini-starkware)

.github/workflows/blockifier_reexecution_ci.yml line 26 at r1 (raw file):

Previously, dorimedini-starkware wrote…

If there's a reason to run blockifier checks, then yes,
but

changes in the blockifier CI are changes in the checks, not changes in the crate, so no reason to trigger reexecution checks

native blockifier is not a dependency of reexecution, so changes in the native blockifier shouldn't trigger reexecution - the blockifier CI also runs native blockifier stuff (actually, only the upload_artifacts workflow should really be triggered on changes to native blockifier, we can probably update the blockifier CI triggers to reflect this).

Done.

.github/workflows/blockifier_reexecution_ci.yml line 31 at r1 (raw file):

Previously, dorimedini-starkware wrote…

why? reexecution doesn't depend on native blockifier

Done.

.github/workflows/blockifier_reexecution_ci.yml line 57 at r1 (raw file):

Previously, dorimedini-starkware wrote…

ah, missed that..

Done.

dorimedini-starkware

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

.github/workflows/blockifier_reexecution_ci.yml line 52 at r1 (raw file):

Previously, aner-starkware wrote…

That's not what I recall; look at your request in the conversation.

discussed offline, we want a secret-less solution..

aner-starkware self-assigned this Nov 6, 2024

aner-starkware force-pushed the aner/reexecution_tests_different_block_versions branch from 28ba6d1 to 61ea9d2 Compare November 6, 2024 12:59

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch from 9b59603 to 87a2d96 Compare November 6, 2024 13:46

aner-starkware force-pushed the aner/reexecution_tests_different_block_versions branch from 61ea9d2 to 41381bb Compare November 6, 2024 15:18

aner-starkware changed the base branch from aner/reexecution_tests_different_block_versions to main November 6, 2024 17:29

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch from 87a2d96 to 0926455 Compare November 18, 2024 11:58

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch from 0926455 to 582d756 Compare November 18, 2024 12:17

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch from 582d756 to 00aabb1 Compare November 18, 2024 12:25

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch from 00aabb1 to eafed7d Compare November 18, 2024 12:40

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch from eafed7d to 7b90002 Compare November 18, 2024 13:03

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch from 7b90002 to f53de53 Compare November 18, 2024 13:12

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch from f53de53 to 2a12aef Compare November 18, 2024 13:20

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch from 2a12aef to b330615 Compare November 18, 2024 13:26

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch from b330615 to 4f7def5 Compare November 19, 2024 08:12

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch from 4f7def5 to 6261b64 Compare November 19, 2024 14:41

aner-starkware requested a review from dorimedini-starkware November 19, 2024 15:05

aner-starkware requested a review from AvivYossef-starkware November 19, 2024 15:06

dorimedini-starkware reviewed Nov 19, 2024

View reviewed changes

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch 2 times, most recently from 1a432c7 to c87d64b Compare November 19, 2024 16:59

aner-starkware commented Nov 19, 2024

View reviewed changes

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch from c87d64b to 5dc406e Compare November 19, 2024 17:06

aner-starkware commented Nov 19, 2024

View reviewed changes

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch from 5dc406e to 6912a1e Compare November 19, 2024 17:23

dorimedini-starkware reviewed Nov 19, 2024

View reviewed changes

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch 2 times, most recently from 4db00dc to d5aae46 Compare November 19, 2024 17:58

chore(blockifier_reexecution): add blockifier reexecution test to ci

bf185f6

aner-starkware force-pushed the aner/add_blockifier_reexecution_to_ci branch from d5aae46 to bf185f6 Compare November 19, 2024 18:04

aner-starkware commented Nov 19, 2024

View reviewed changes

dorimedini-starkware reviewed Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(blockifier_reexecution): add blockifier reexecution test to ci #1857

chore(blockifier_reexecution): add blockifier reexecution test to ci #1857

aner-starkware commented Nov 6, 2024

reviewable-StarkWare commented Nov 6, 2024

github-actions bot commented Nov 6, 2024

github-actions bot commented Nov 6, 2024

github-actions bot commented Nov 18, 2024

codecov bot commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 19, 2024

github-actions bot commented Nov 19, 2024

aner-starkware commented Nov 19, 2024

dorimedini-starkware left a comment

dorimedini-starkware left a comment

aner-starkware left a comment

aner-starkware commented Nov 19, 2024

aner-starkware left a comment

aner-starkware left a comment

aner-starkware commented Nov 19, 2024

dorimedini-starkware left a comment

dorimedini-starkware left a comment

aner-starkware commented Nov 19, 2024

dorimedini-starkware left a comment

aner-starkware commented Nov 19, 2024

dorimedini-starkware left a comment

aner-starkware left a comment

dorimedini-starkware left a comment

chore(blockifier_reexecution): add blockifier reexecution test to ci #1857

Are you sure you want to change the base?

chore(blockifier_reexecution): add blockifier reexecution test to ci #1857

Conversation

aner-starkware commented Nov 6, 2024

reviewable-StarkWare commented Nov 6, 2024

github-actions bot commented Nov 6, 2024

github-actions bot commented Nov 6, 2024

github-actions bot commented Nov 18, 2024

codecov bot commented Nov 18, 2024 • edited Loading

Codecov Report

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 19, 2024

github-actions bot commented Nov 19, 2024

aner-starkware commented Nov 19, 2024

dorimedini-starkware left a comment

Choose a reason for hiding this comment

dorimedini-starkware left a comment

Choose a reason for hiding this comment

aner-starkware left a comment

Choose a reason for hiding this comment

aner-starkware commented Nov 19, 2024

aner-starkware left a comment

Choose a reason for hiding this comment

aner-starkware left a comment

Choose a reason for hiding this comment

aner-starkware commented Nov 19, 2024

dorimedini-starkware left a comment

Choose a reason for hiding this comment

dorimedini-starkware left a comment

Choose a reason for hiding this comment

aner-starkware commented Nov 19, 2024

dorimedini-starkware left a comment

Choose a reason for hiding this comment

aner-starkware commented Nov 19, 2024

dorimedini-starkware left a comment

Choose a reason for hiding this comment

aner-starkware left a comment

Choose a reason for hiding this comment

dorimedini-starkware left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 18, 2024 •

edited

Loading