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

chore(blockifier): verify local cairo repo state before compiling #194

Conversation

dorimedini-starkware
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware commented Jul 30, 2024

This change is Reviewable

@dorimedini-starkware dorimedini-starkware self-assigned this Jul 30, 2024
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from 0516f8b to 249b887 Compare July 30, 2024 13:21
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from e3b2330 to e62e313 Compare July 30, 2024 13:21
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from 249b887 to d870356 Compare July 31, 2024 16:34
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from e62e313 to 4fe6734 Compare July 31, 2024 16:34
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from d870356 to 13acfad Compare July 31, 2024 16:42
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from 4fe6734 to 9facef4 Compare July 31, 2024 16:42
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from 13acfad to aa0605e Compare July 31, 2024 16:51
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from 9facef4 to 0d099c5 Compare July 31, 2024 16:52
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from aa0605e to 9ae0de0 Compare July 31, 2024 16:54
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from 0d099c5 to a12e845 Compare July 31, 2024 16:54
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from 9ae0de0 to 66e9a05 Compare August 1, 2024 08:50
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from a12e845 to d2dd1b8 Compare August 1, 2024 08:50
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from 66e9a05 to 8cbaa12 Compare August 1, 2024 08:53
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from d2dd1b8 to 10d4789 Compare August 1, 2024 08:53
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from 8cbaa12 to 1fe0755 Compare August 1, 2024 09:16
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from 55fda8f to ca9ccb1 Compare August 5, 2024 12:59
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from 55907ae to 031e4da Compare August 5, 2024 13:00
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from ca9ccb1 to 3ec4eb7 Compare August 6, 2024 12:16
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from 031e4da to 6e5f836 Compare August 6, 2024 12:16
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from 3ec4eb7 to ef4ccf9 Compare August 6, 2024 13:29
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from 6e5f836 to 9942ae2 Compare August 6, 2024 13:30
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from ef4ccf9 to 7ce475e Compare August 7, 2024 14:51
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from 9942ae2 to 0895bc5 Compare August 7, 2024 14:51
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from 7ce475e to cbb6cfe Compare August 7, 2024 16:16
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from 0895bc5 to 388820c Compare August 7, 2024 16:16
@dorimedini-starkware dorimedini-starkware marked this pull request as ready for review August 8, 2024 09:51
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from cbb6cfe to 28ecdcd Compare August 11, 2024 07:45
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from 388820c to 7a76151 Compare August 11, 2024 07:45
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from 28ecdcd to 9b19d4e Compare August 11, 2024 08:55
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from 7a76151 to 83dc987 Compare August 11, 2024 08:55
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from 9b19d4e to aa4a115 Compare August 11, 2024 09:35
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from 83dc987 to b29a56b Compare August 11, 2024 09:35
Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware, @dorimedini-starkware, @meship-starkware, and @nimrod-starkware)


crates/blockifier/src/test_utils/cairo_compile.rs line 183 at r4 (raw file):

        "Cannot verify Cairo1 contracts, Cairo repo not found at {0}.\n\
        Please run:\n\
        git clone https://github.com/starkware-libs/cairo {0} && git -C {0} checkout {1}\n\

The script checkout in the next stage, I think this can be removed


crates/blockifier/src/test_utils/cairo_compile.rs line 205 at r4 (raw file):

        "--verify",
        &tag,
    ]));

What does this add to the previous action (I mean, if the tag doesn't exist, we will fail in the checkout stage)?

Code quote:

    // Verify that the checked out tag is as expected.
    run_and_verify_output(Command::new("git").args([
        "-C",
        cairo_repo_path.to_str().unwrap(),
        "rev-parse",
        "--verify",
        &tag,
    ]));

Copy link
Collaborator Author

@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 @AvivYossef-starkware, @meship-starkware, @nimrod-starkware, and @TzahiTaub)


crates/blockifier/src/test_utils/cairo_compile.rs line 205 at r4 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

What does this add to the previous action (I mean, if the tag doesn't exist, we will fail in the checkout stage)?

in case we somehow "silently" fail the checkout and not catch the error.
may be redundant, but when we parallelize this test (takes forever to sequencially compile all contracts...) it will be useful to split checkout / verify checkout phases anyway

@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from aa4a115 to cf43b14 Compare August 11, 2024 12:58
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from b29a56b to 9d6f7ff Compare August 11, 2024 12:58
Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware, @meship-starkware, and @nimrod-starkware)


crates/blockifier/src/test_utils/cairo_compile.rs line 205 at r4 (raw file):

Previously, dorimedini-starkware wrote…

in case we somehow "silently" fail the checkout and not catch the error.
may be redundant, but when we parallelize this test (takes forever to sequencially compile all contracts...) it will be useful to split checkout / verify checkout phases anyway

Can't say I understand (to parallelize I assume you need to run different tag compilation sequentially and parallelize only contracts using the same tag. In this case you can make one checkout, parallel compilations, and when finished - new checkout, (parallel) compilation etc. Don't see where the verify helps here. But unblocking

Copy link
Collaborator Author

@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 @AvivYossef-starkware, @meship-starkware, and @nimrod-starkware)


crates/blockifier/src/test_utils/cairo_compile.rs line 205 at r4 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Can't say I understand (to parallelize I assume you need to run different tag compilation sequentially and parallelize only contracts using the same tag. In this case you can make one checkout, parallel compilations, and when finished - new checkout, (parallel) compilation etc. Don't see where the verify helps here. But unblocking

exactly.
I can checkout once, and verify in every thread

Copy link
Collaborator Author

dorimedini-starkware commented Aug 11, 2024

Merge activity

@dorimedini-starkware dorimedini-starkware changed the base branch from 07-29-feat_blockifier_support_cairo1_local_recompilation to graphite-base/194 August 11, 2024 16:10
@dorimedini-starkware dorimedini-starkware changed the base branch from graphite-base/194 to main August 11, 2024 16:23
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_verify_local_cairo_repo_state_before_compiling branch from 9d6f7ff to e31d80c Compare August 11, 2024 16:24
@dorimedini-starkware dorimedini-starkware merged commit fecdc0c into main Aug 11, 2024
14 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 13, 2024
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.

2 participants