-
Notifications
You must be signed in to change notification settings - Fork 186
Stav/remove test compare to python vm #2086
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: starkware-development
Are you sure you want to change the base?
Stav/remove test compare to python vm #2086
Conversation
|
Benchmark Results for unmodified programs 🚀
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## starkware-development #2086 +/- ##
=========================================================
- Coverage 96.62% 96.55% -0.08%
=========================================================
Files 102 102
Lines 44388 43250 -1138
=========================================================
- Hits 42889 41759 -1130
+ Misses 1499 1491 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hi @Stavbe! Is there a way to keep the memory comparison anyway? Comparing with cairo-lang is a great way for testing the Cairo VM, and ensuring that we don't break compatibility. We came up with two solutions:
|
Another alternative could be to add a script that fills the memory holes when necessary, and leaving the VM output without holes. Would this work? |
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 6 of 6 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Stavbe)
62792f5
to
87648df
Compare
87648df
to
69efbc5
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.
Hi @JulianGCalderon,
We moved the hole-filling logic to run only in proof mode, so I updated this PR to perform memory comparison checks only in the other cases.
Reviewable status: 1 of 7 files reviewed, 2 unresolved discussions (waiting on @DavidLevitGurevich and @Stavbe)
vm/src/tests/compare_outputs_dynamic_layouts.sh
line 197 at r3 (raw file):
echo "Running cairo-lang with case: $case" cairo-run --program "$full_program" \ --layout "dynamic" --cairo_layout_params_file "$full_layout" --proof_mode \
proof mode
vm/src/tests/compare_factorial_outputs_all_layouts.sh
line 14 at r3 (raw file):
# Run cairo_lang echo "Running cairo_lang with layout $layout" cairo-run --layout $layout --proof_mode --program $factorial_compiled --trace_file factorial_py.trace --memory_file factorial_py.memory --air_public_input factorial_py.air_public_input --air_private_input factorial_py.air_private_input
proof mode
Initially, we planned to fill holes only in the memory used for prover_input_info, but we realized it would be better for Stone to receive a memory file without holes as well —since this they are already computed by the VM.
Therefore, we no longer intend to compare the memory against the Python VM’s memory, as they will not be identical.
This change is