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

chore(cli): Forward nargo execute to noir_artifact_cli #7406

Open
wants to merge 16 commits into
base: 7381-use-artifact-fs
Choose a base branch
from

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Feb 17, 2025

Description

Problem*

Resolves #7380

Builds on #7391

Summary*

Changed nargo_cli::execute_cmd::run to create and run an instance of noir_artifact_cli::commands::execute_cmd::ExecuteCommand to completely outsource program execution to noir_artifact_cli.

Additional Context

Up to now the ExecuteCommand in noir_artifact_cli was part of the binary package; now it's exposed as part of the library through the commands module, so we can forward calls from nargo by using it as a dependency and just mapping command parameters.

Other options would be to:

  • "shell out" to the noir-artifact execute as a sub-process
  • use the functions in noir_artifact_cli::executions

The reason I thought creating and running ExecuteCommand would be good are:

  • With the sub-process option:
    • Aztec packages would need to install both binaries and make nargo aware of where noir-artifact can be found, which requires changing a bunch of scripts that currently use nargo
    • We would lose the static typing checks around parameter passing, instead we would need to pass on the subset of CLI options that noir-artefact execute expects, which is a bit more error prone
    • We wouldn't be able to pass on custom foreign executors generically, although once we can handle oracle transcripts we might want nargo execute to work with them as well, and there wouldn't be a need to differentiate.
    • The locking handled by nargo has to span the compilation and the execution phase; if they are in one process there cannot be any clashes, although at the moment noir-artifact doesn't do any locking
  • With the executions option there is still some duplication of the workflow, and we're a bit more removed from the conceptual "shelling out"

I mainly wanted to leave aztec-packages untouched for now, but with this change at least it would be simpler as nargo does little more than call the library functions to do the work.

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo aartifact on default settings.

@aakoshh aakoshh requested a review from a team February 17, 2025 12:41
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: c5d993a Previous: 119bf62 Ratio
global_var_regression_entry_points 0.618 s 0.506 s 1.22

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Compilation Memory'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 66bcf73 Previous: b7ace68 Ratio
rollup-base-private 1170 MB 949.5 MB 1.23
rollup-base-public 1040 MB 815.02 MB 1.28
rollup-block-merge 596.48 MB 371.19 MB 1.61
rollup-block-root-empty 550.39 MB 325.09 MB 1.69
rollup-merge 548.8 MB 323.5 MB 1.70
rollup-root 596.49 MB 371.18 MB 1.61

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Memory'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 66bcf73 Previous: b7ace68 Ratio
rollup-base-private 655.58 MB 430.27 MB 1.52
rollup-base-public 594.32 MB 369.01 MB 1.61
rollup-block-merge 540.64 MB 315.32 MB 1.71
rollup-merge 533.22 MB 307.91 MB 1.73
rollup-root 540.62 MB 315.3 MB 1.71

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 7f44036 Previous: fdfe2bf Ratio
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob 66 s 51 s 1.29

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@aakoshh aakoshh changed the title chore(cli): Forward nargo execute to noir_artifact_cli::execution functions chore(cli): Forward nargo execute to noir_artifact_cli Feb 17, 2025
@aakoshh aakoshh force-pushed the 7380-nargo-exec-fwd branch from 64e185e to 66bcf73 Compare February 17, 2025 14:27
@aakoshh
Copy link
Contributor Author

aakoshh commented Feb 17, 2025

Hm, I don't understand why the compilation memory would be affected by this PR 🤔
The execution memory as well, I'm still calling the same things from nargo ops.

I checked locally for example this one:

./target/release/nargo --program-dir ../aztec-packages/noir-projects/noir-protocol-circuits/crates/rollup-block-root-empty compile  --silence-warnings --force

which reports a 70% increase in compilation memory above. For me master and this branch are identical.

Also tried execution with the same identical result:

./target/release/nargo --program-dir ../aztec-packages/noir-projects/noir-protocol-circuits/crates/rollup-block-merge execute --silence-warnings

@aakoshh aakoshh linked an issue Feb 17, 2025 that may be closed by this pull request
@aakoshh aakoshh force-pushed the 7380-nargo-exec-fwd branch from c85cee3 to 42acd9a Compare February 17, 2025 20:55
@aakoshh aakoshh force-pushed the 7380-nargo-exec-fwd branch from 42acd9a to 9c2c3d4 Compare February 17, 2025 20:57
@aakoshh aakoshh mentioned this pull request Feb 17, 2025
5 tasks
@aakoshh aakoshh requested a review from TomAFrench February 19, 2025 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call the artifact CLI from nargo execute
1 participant