-
Notifications
You must be signed in to change notification settings - Fork 65
blockifier: add wait_on_native_compilation flag to test get compiled class caching #10493
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
base: main-v0.14.1
Are you sure you want to change the base?
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
b71a015 to
f78ae7f
Compare
8662f9d to
b1c40c5
Compare
f78ae7f to
b4ccc10
Compare
b1c40c5 to
d24c18a
Compare
ArniStarkware
left a comment
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 1 files reviewed, 1 unresolved discussion (waiting on @avivg-starkware, @noaov1, and @TzahiTaub)
a discussion (no related file):
See this discussion:
https://stackoverflow.com/questions/31195529/escaping-commas-in-macro-output
TzahiTaub
left a comment
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 1 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @avivg-starkware, and @noaov1)
crates/blockifier/src/state/state_reader_and_contract_manager_test.rs line 168 at r1 (raw file):
is_declared_result: None, }, expected_result: Ok(RunnableCompiledClass::test_casm_contract_class()),
IIUC, when the test runs with native, it tries to compile the contract, fails (as the sierra code is invalid, maybe there a re more reasons), puts in the cache a value of CompiledClasses::V1Native(CachedCairoNative::CompilationFailed(CompiledClassV1)), and when it returns via the get_compiled_class (using the to_runnable) it's returned as a regular casm so the test succeeds. I'm not sure this test should test the native class manager, the dedicated test above might be enough, but the current flow for the native runs hides unexpected behavior (specifically, if we replace the empty sierra with the right one this test should fail). Need to think how to make the test succeed in native, or close it for native entirely
Code quote:
expected_result: Ok(RunnableCompiledClass::test_casm_contract_class()),
TzahiTaub
left a comment
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 1 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @avivg-starkware, and @noaov1)
crates/blockifier/src/state/state_reader_and_contract_manager_test.rs line 168 at r1 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
IIUC, when the test runs with native, it tries to compile the contract, fails (as the sierra code is invalid, maybe there a re more reasons), puts in the cache a value of
CompiledClasses::V1Native(CachedCairoNative::CompilationFailed(CompiledClassV1)), and when it returns via the get_compiled_class (using theto_runnable) it's returned as a regular casm so the test succeeds. I'm not sure this test should test the native class manager, the dedicated test above might be enough, but the current flow for the native runs hides unexpected behavior (specifically, if we replace the empty sierra with the right one this test should fail). Need to think how to make the test succeed in native, or close it for native entirely
I think that if the cache is tested for native in the previous test (i.e., that we get CachedCairoNative::Compiled from the the manager for appropriate cairo 1), we can just put this entire test under #[cfg(not(feature = "cairo_native"))]
b4ccc10 to
1823079
Compare
d24c18a to
84c4a08
Compare
1823079 to
9876970
Compare
84c4a08 to
2d30889
Compare
TzahiTaub
left a comment
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 1 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @avivg-starkware, and @noaov1)
crates/blockifier/src/state/state_reader_and_contract_manager_test.rs line 175 at r2 (raw file):
GetCompiledClassTestScenario { expectations: GetCompiledClassTestExpectation { get_compiled_classes_result: Some(Ok(CompiledClasses::from_runnable_for_testing(
As long as this function returns SierraContractClass::default() for the V1, the native feature cannot be tested properly in the below unit test.
Code quote:
from_runnable_for_testing(
TzahiTaub
left a comment
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.
@TzahiTaub reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @avivg-starkware, and @noaov1)
9876970 to
8918ab4
Compare
2d30889 to
8c5631d
Compare
8918ab4 to
d7866c5
Compare
8c5631d to
61bf8f1
Compare
61bf8f1 to
c3f6aae
Compare
c3f6aae to
8c2c56e
Compare

No description provided.