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

feat(blockifier): support Cairo1 local recompilation #184

Conversation

dorimedini-starkware
Copy link
Collaborator

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

This change is Reviewable

@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch 2 times, most recently from d79d774 to f152b74 Compare July 30, 2024 08:17
Copy link

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.682 ms 35.105 ms 35.602 ms]
change: [+1.3730% +2.5866% +3.9352%] (p = 0.00 < 0.05)
Performance has regressed.
Found 14 outliers among 100 measurements (14.00%)
6 (6.00%) high mild
8 (8.00%) high severe

@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from e736fb7 to 0b4fda1 Compare July 30, 2024 08:37
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from f152b74 to 0516f8b Compare July 30, 2024 08:37
Copy link

codecov bot commented Jul 30, 2024

Codecov Report

Attention: Patch coverage is 0% with 47 lines in your changes missing coverage. Please review.

Please upload report for BASE (07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_@b4e9649). Learn more about missing BASE report.

Files Patch % Lines
crates/blockifier/src/test_utils/cairo_compile.rs 0.00% 37 Missing ⚠️
crates/blockifier/src/test_utils/contracts.rs 0.00% 10 Missing ⚠️
Additional details and impacted files
@@                                          Coverage Diff                                           @@
##             07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_     #184   +/-   ##
======================================================================================================
  Coverage                                                                       ?   76.24%           
======================================================================================================
  Files                                                                          ?      330           
  Lines                                                                          ?    35104           
  Branches                                                                       ?    35104           
======================================================================================================
  Hits                                                                           ?    26765           
  Misses                                                                         ?     6045           
  Partials                                                                       ?     2294           

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

@dorimedini-starkware dorimedini-starkware self-assigned this Jul 30, 2024
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from 0b4fda1 to d7806fc Compare July 30, 2024 13:21
@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-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from d7806fc to b7301ad Compare July 31, 2024 16:34
@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-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from b7301ad to 8e54eec Compare July 31, 2024 16:42
@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-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from 8e54eec to 1927450 Compare July 31, 2024 16:51
@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-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from 1927450 to 5d0ac45 Compare July 31, 2024 16:54
@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
Copy link

github-actions bot commented Aug 7, 2024

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.697 ms 65.781 ms 65.874 ms]
change: [-9.6039% -6.0570% -2.8548%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severe

@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from 378e67b to e57ad1c Compare August 7, 2024 16:16
@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
Copy link

github-actions bot commented Aug 7, 2024

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.565 ms 65.645 ms 65.736 ms]
change: [-9.9533% -6.5400% -3.5059%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
2 (2.00%) high mild
3 (3.00%) high severe

@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-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from e57ad1c to 1fd4711 Compare August 11, 2024 07:45
@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
Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.924 ms 65.031 ms 65.149 ms]
change: [-10.420% -7.3910% -4.7155%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe

@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from 1fd4711 to 678e67e Compare August 11, 2024 08:55
@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
Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.746 ms 64.838 ms 64.950 ms]
change: [-7.4846% -4.1411% -1.3395%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) high mild
2 (2.00%) high severe

@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from 678e67e to 469c6a8 Compare August 11, 2024 09:35
@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
Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.607 ms 64.675 ms 64.757 ms]
change: [-10.881% -7.0324% -3.7081%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) high mild
2 (2.00%) high severe

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 5 of 5 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware, @dorimedini-starkware, @meship-starkware, and @noaov1)


crates/blockifier/src/test_utils/cairo_compile.rs line 174 at r9 (raw file):

}

fn verify_cairo1_compiler_deps(git_tag_override: Option<String>) {

verify sounds like a no-side-effect action.

Suggestion:

prepare_cairo1_compiler_deps

crates/blockifier/src/test_utils/contracts.rs line 279 at r9 (raw file):

                    Self::LegacyTestContract => (
                        // Legacy contract requires specific compiler tag (which is the point of
                        // the test contract), + to build the compiler an

I don't understand the sentence (== this is why the test contract exists?)

Suggestion:

which is the point of the test contract

crates/blockifier/src/test_utils/contracts.rs line 282 at r9 (raw file):

                        // older rust version is required.
                        Some(LEGACY_CONTRACT_COMPILER_TAG.into()),
                        Some(String::from("2023-07-05")),

Why 1 is const and the other isn't?

Code quote:

                        Some(LEGACY_CONTRACT_COMPILER_TAG.into()),
                        Some(String::from("2023-07-05")),

@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from 469c6a8 to b4e9649 Compare August 11, 2024 12:58
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, @noaov1, and @TzahiTaub)


crates/blockifier/src/test_utils/contracts.rs line 279 at r9 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

I don't understand the sentence (== this is why the test contract exists?)

better?

@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
Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.773 ms 64.859 ms 64.958 ms]
change: [-9.6684% -6.5893% -3.9304%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
2 (2.00%) high mild
2 (2.00%) high severe

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.

:lgtm:

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


crates/blockifier/src/test_utils/contracts.rs line 279 at r9 (raw file):

Previously, dorimedini-starkware wrote…

better?

Much

Copy link
Collaborator Author

dorimedini-starkware commented Aug 11, 2024

Merge activity

@dorimedini-starkware dorimedini-starkware changed the base branch from 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ to graphite-base/184 August 11, 2024 16:01
Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.533 ms 64.595 ms 64.666 ms]
change: [-2.5929% -1.8449% -1.1593%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high severe

@dorimedini-starkware dorimedini-starkware changed the base branch from graphite-base/184 to main August 11, 2024 16:09
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_support_cairo1_local_recompilation branch from cf43b14 to 8d3dae6 Compare August 11, 2024 16:10
Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [64.474 ms 64.528 ms 64.596 ms]
change: [-8.6383% -5.3171% -2.4405%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severe

@dorimedini-starkware dorimedini-starkware merged commit e41bfdf into main Aug 11, 2024
25 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 13, 2024
This pull request was closed.
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.

3 participants