refactor: add function_runner feature flag and CairoFunctionRunner alias#2377
refactor: add function_runner feature flag and CairoFunctionRunner alias#2377naor-starkware wants to merge 4 commits intomainfrom
Conversation
- Add lean `function_runner = []` feature flag (zero extra deps) - `test_utils` now implies `function_runner` - Gate `function_runner` module on new flag instead of `test_utils` - Make module and `run_from_entrypoint`/`get_function_pc` `pub` - Add `CairoFunctionRunner` type alias for `CairoRunner` - Remove spurious `#[allow(dead_code)]` from `EntryPoint` - Update module doc comment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2377 +/- ##
=======================================
Coverage 96.07% 96.07%
=======================================
Files 105 105
Lines 37737 37737
=======================================
Hits 36254 36254
Misses 1483 1483 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
OmriEshhar1
left a comment
There was a problem hiding this comment.
@OmriEshhar1 reviewed 3 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on efrat-starkware and YairVaknin-starkware).
|
Benchmark Results for unmodified programs 🚀
|
OmriEshhar1
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on efrat-starkware and YairVaknin-starkware).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
OmriEshhar1
left a comment
There was a problem hiding this comment.
@OmriEshhar1 reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on efrat-starkware and YairVaknin-starkware).
OmriEshhar1
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on efrat-starkware and YairVaknin-starkware).
Yael-Starkware
left a comment
There was a problem hiding this comment.
@Yael-Starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on efrat-starkware, naor-starkware, and YairVaknin-starkware).
vm/Cargo.toml line 24 at r2 (raw file):
# Note that these features are not retro-compatible with the cairo Python VM. function_runner = []
why do you need a separate feature flag for this?
Code quote:
function_runner = []|
Previously, Yael-Starkware (YaelD) wrote…
|
Yael-Starkware
left a comment
There was a problem hiding this comment.
@Yael-Starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on efrat-starkware, naor-starkware, and YairVaknin-starkware).
vm/Cargo.toml line 24 at r2 (raw file):
Previously, naor-starkware wrote…
function_runnerneeds to be exposed toproving-utils, so instead of pulling in everything undertest_utils, I added a separate feature flag that exposes only what's relevant tofunction_runner
is it needed by proving utils only for testing?
Yael-Starkware
left a comment
There was a problem hiding this comment.
@Yael-Starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on efrat-starkware, naor-starkware, and YairVaknin-starkware).
vm/Cargo.toml line 24 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
is it needed by proving utils only for testing?
can you explain the dependency graph here?
who uses test_utils and who uses function_runner?
|
Previously, Yael-Starkware (YaelD) wrote…
test_utils implies function_runner - enabling test_utils automatically enables function_runner. You can enable function_runner alone function_runner Nobody directly. No crate in the workspace enables it alone. It only gets activated transitively when test_utils is enabled. It exists as a standalone flag so that external consumers (e.g. Starknet ,Proving-utlis) can use just CairoFunctionRunner + test_helpers |
|
This infrastructure is only needed for tests. If proving-utils needs it, it would only be for test usage. |
Yael-Starkware
left a comment
There was a problem hiding this comment.
@Yael-Starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on efrat-starkware, naor-starkware, and YairVaknin-starkware).
vm/Cargo.toml line 24 at r2 (raw file):
Previously, naor-starkware wrote…
is it needed by proving utils only for testing?
This infrastructure is only needed for tests. If proving-utils needs it, it would only be for test usage.
If both flags are for testing only, please drop function_runner for simplicity.
This separation is just more flags for users to discover and manage.
function_runner was only ever used as a test-only building block. Since both flags serve testing purposes only, keeping function_runner as a separate flag adds unnecessary complexity for users. test_utils now directly gates CairoFunctionRunner and test_helpers instead of implying function_runner. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Yael-Starkware
left a comment
There was a problem hiding this comment.
@Yael-Starkware made 2 comments.
Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on efrat-starkware, naor-starkware, OmriEshhar1, and YairVaknin-starkware).
vm/src/vm/runners/function_runner.rs line 4 at r3 (raw file):
//! //! Provides a simplified API for executing individual Cairo 0 functions by name or PC. //! Enabled by the `function_runner` feature flag.
remove.
Code quote:
//! Enabled by the `function_runner` feature flag.vm/src/vm/runners/function_runner.rs line 23 at r3 (raw file):
/// Type alias for [`CairoRunner`] with testing methods enabled. /// Mirrors the Python `CairoFunctionRunner` class interface. pub type CairoFunctionRunner = CairoRunner;
why do you need another type?
Code quote:
/// Type alias for [`CairoRunner`] with testing methods enabled.
/// Mirrors the Python `CairoFunctionRunner` class interface.
pub type CairoFunctionRunner = CairoRunner;
Yael-Starkware
left a comment
There was a problem hiding this comment.
@Yael-Starkware resolved 1 discussion.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on efrat-starkware, naor-starkware, OmriEshhar1, and YairVaknin-starkware).
|
Previously, Yael-Starkware (YaelD) wrote…
It's a semantic alias, not a technical one. CairoRunner is the general execution engine. CairoFunctionRunner communicates that |
Yael-Starkware
left a comment
There was a problem hiding this comment.
@Yael-Starkware made 1 comment.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on efrat-starkware, naor-starkware, OmriEshhar1, and YairVaknin-starkware).
vm/src/vm/runners/function_runner.rs line 23 at r3 (raw file):
Previously, naor-starkware wrote…
It's a semantic alias, not a technical one. CairoRunner is the general execution engine. CairoFunctionRunner communicates that
you're using it specifically to invoke individual functions by name/PC - the same way the Python CairoFunctionRunner did. Without the
alias, every test file looks identical whether it's running a full program or calling a function entry point.
not sure I understand the issue, the function name indicates what it is doing.
besides, how do you enforce using the correct type?
Yael-Starkware
left a comment
There was a problem hiding this comment.
@Yael-Starkware made 1 comment.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on efrat-starkware, naor-starkware, OmriEshhar1, and YairVaknin-starkware).
vm/src/vm/runners/function_runner.rs line 23 at r3 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
not sure I understand the issue, the function name indicates what it is doing.
besides, how do you enforce using the correct type?
as we discussed on huddle, since we are deleting the python-vm , no need to keep it's naming conventions. so please remove this type.
YairVaknin-starkware
left a comment
There was a problem hiding this comment.
@YairVaknin-starkware reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on efrat-starkware and naor-starkware).
CHANGELOG.md line 15 at r3 (raw file):
#### Upcoming Changes * refactor: add `CairoFunctionRunner` type alias for `CairoRunner` under the `test_utils` feature flag [#2377](https://github.com/starkware-libs/cairo-vm/pull/2377)
description should probably be adjusted to the current state of the PR (after Yael's comment)
Code quote:
* refactor: add `CairoFunctionRunner` type alias for `CairoRunner` under the `test_utils` feature flag [#2377](https://github.com/starkware-libs/cairo-vm/pull/2377)vm/src/vm/runners/function_runner.rs line 22 at r3 (raw file):
/// Identifies a Cairo function entrypoint either by function name or by program counter. #[allow(dead_code)]
safe to remove? I don't see Pc ever constructed outside of test? Maybe just mark this variant with the attr?
Code quote:
#[allow(dead_code)]
TITLE
Description
Description of the pull request changes and motivation.
Checklist
This change is