-
Notifications
You must be signed in to change notification settings - Fork 25
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
refactor(blockifier): split feature contracts to groups based on their… #2236
Conversation
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2236 +/- ##
===========================================
+ Coverage 40.10% 71.34% +31.24%
===========================================
Files 26 102 +76
Lines 1895 13636 +11741
Branches 1895 13636 +11741
===========================================
+ Hits 760 9729 +8969
- Misses 1100 3493 +2393
- Partials 35 414 +379 ☔ View full report in Codecov by Sentry. |
341c9dd
to
4d28c76
Compare
Artifacts upload triggered. View details here |
4d28c76
to
3acb443
Compare
Artifacts upload workflows: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)
crates/blockifier/tests/feature_contracts_compatibility_test.rs
line 46 at r1 (raw file):
prepare_group_tag_compiler_deps(tag_override.clone()); } for contract in feature_contracts
this loop will be parallelized?
Code quote:
for contract in feature_contracts
crates/blockifier/src/test_utils/contracts.rs
line 327 at r1 (raw file):
&self, tag_override: Option<String>, cargo_nightly_arg: Option<String>,
this pair appears often in this PR; please create a TagAndToolchain
struct containing these, and use it
- in function signatures,
- as function return values (where relevant),
- as the key type on your
TagToContractsMapping
type
Code quote:
tag_override: Option<String>,
cargo_nightly_arg: Option<String>,
crates/blockifier/src/test_utils/contracts.rs
line 328 at r1 (raw file):
tag_override: Option<String>, cargo_nightly_arg: Option<String>, ) -> Vec<u8> {
why do you inject this input now?
tag + nightly arg is still a property of the enum variant; you can still use the fixed_tag_and_rust_toolchain
to get it.
you can also use fixed_tag_and_rust_toolchain
in the implementation of feature_contracts_by_tag
Code quote:
pub fn compile(
&self,
tag_override: Option<String>,
cargo_nightly_arg: Option<String>,
) -> Vec<u8> {
crates/blockifier/src/test_utils/contracts.rs
line 425 at r1 (raw file):
} let tag_and_version = contract.fixed_tag_and_rust_toolchain(); let contracts_to_insert = if contract.has_two_versions() {
what does this function mean, now that there are 3 possible versions (with native)?
this function should indicate cairo1 vs cairo0, and your parallelization feature is for cairo1 only
Code quote:
if contract.has_two_versions()
crates/blockifier/src/test_utils/contracts.rs
line 427 at r1 (raw file):
let contracts_to_insert = if contract.has_two_versions() { let mut other_contract = contract; other_contract.set_cairo_version(contract.cairo_version().other());
how is this defined...?
Code quote:
cairo_version().other()
crates/blockifier/src/test_utils/contracts.rs
line 435 at r1 (raw file):
.entry(tag_and_version) .or_insert_with(Vec::new) .extend(contracts_to_insert);
why add cairo0 contracts to this list...?
Code quote:
.extend(contracts_to_insert);
crates/blockifier/src/test_utils/cairo_compile.rs
line 232 at r1 (raw file):
} fn prepare_cairo1_compiler_deps(git_tag_override: Option<String>) {
this function makes no changes to the state of the machine it's running on, so I would rename
Suggestion:
fn verify_cairo1_compiler_deps
There was a problem hiding this 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, 8 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)
crates/blockifier/src/test_utils/contracts.rs
line 425 at r1 (raw file):
Previously, dorimedini-starkware wrote…
what does this function mean, now that there are 3 possible versions (with native)?
this function should indicate cairo1 vs cairo0, and your parallelization feature is for cairo1 only
I thought about having two Types: one Cairo version and the second completion type (the names are not set). I also thought about addressing it in a different PR, where I will remove the Cairo Native folder.
https://reviewable.io/reviews/starkware-libs/sequencer/1301#-O93wpyO6HNmUICVLokU
crates/blockifier/tests/feature_contracts_compatibility_test.rs
line 46 at r1 (raw file):
Previously, dorimedini-starkware wrote…
this loop will be parallelized?
That is the intention. For now, our priority is to save the Sierra files and remove the native folder, so I don't know when it will happen.
crates/blockifier/src/test_utils/contracts.rs
line 416 at r1 (raw file):
} pub fn feature_contracts_by_tag() -> TagToContractsMapping {
I decided to create a new function because all_contracts returns an iterator of all the contracts, and then all feature contracts filter the erc_20 contract, so I didn't know if I could remove the ERC20 in all contracts. Also, there is another use that needs the iterator form
Code quote:
fn feature_contracts_by_tag() -> TagToContractsMapping {
3acb443
to
db4eebe
Compare
There was a problem hiding this 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 3 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)
crates/blockifier/src/test_utils/cairo_compile.rs
line 232 at r1 (raw file):
Previously, dorimedini-starkware wrote…
this function makes no changes to the state of the machine it's running on, so I would rename
Done.
crates/blockifier/src/test_utils/contracts.rs
line 327 at r1 (raw file):
Previously, dorimedini-starkware wrote…
this pair appears often in this PR; please create a
TagAndToolchain
struct containing these, and use it
- in function signatures,
- as function return values (where relevant),
- as the key type on your
TagToContractsMapping
type
Done.
crates/blockifier/src/test_utils/contracts.rs
line 328 at r1 (raw file):
Previously, dorimedini-starkware wrote…
why do you inject this input now?
tag + nightly arg is still a property of the enum variant; you can still use thefixed_tag_and_rust_toolchain
to get it.
you can also usefixed_tag_and_rust_toolchain
in the implementation offeature_contracts_by_tag
Done.
crates/blockifier/src/test_utils/contracts.rs
line 435 at r1 (raw file):
Previously, dorimedini-starkware wrote…
why add cairo0 contracts to this list...?
Done.
crates/blockifier/tests/feature_contracts_compatibility_test.rs
line 48 at r2 (raw file):
} } _ => {
I don't like the implicit check here, but doing it explicitly will force me to duplicate the cairo1 behavior because we have the feature flag. It won't be necessary with Doris' changes, so maybe I can keep it and add a TODO. WDYT?
a00bb03
to
51c0986
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)
crates/blockifier/tests/feature_contracts_compatibility_test.rs
line 48 at r2 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
I don't like the implicit check here, but doing it explicitly will force me to duplicate the cairo1 behavior because we have the feature flag. It won't be necessary with Doris' changes, so maybe I can keep it and add a TODO. WDYT?
keep it and add a TODO sgtm :)
crates/blockifier/src/test_utils/contracts.rs
line 439 at r3 (raw file):
pub fn cairo1_feature_contracts_by_tag() -> TagToContractsMapping { // EnumIter iterates over all variants with Default::default() as the cairo // version.
delete, not relevant here (you are not using Self::iter()
)
Code quote:
// EnumIter iterates over all variants with Default::default() as the cairo
// version.
51c0986
to
c82646b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @Yoni-Starkware)
crates/blockifier/src/test_utils/contracts.rs
line 439 at r3 (raw file):
Previously, dorimedini-starkware wrote…
delete, not relevant here (you are not using
Self::iter()
)
Done.
There was a problem hiding this 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 r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yoni-Starkware)
… tags