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

Merged
merged 70 commits into from
Feb 28, 2025

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-execute 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-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-execute doesn't do any locking
  • With the execution functions 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 very little on its own.

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
@aakoshh aakoshh requested a review from TomAFrench February 19, 2025 18:53
Base automatically changed from 7381-use-artifact-fs to master February 26, 2025 19:29
@aakoshh aakoshh requested review from TomAFrench and removed request for TomAFrench February 27, 2025 09:49
Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

A little concerned that we're building up a number of redundant versions of ProgramArtifact but we can deal with that later.

@TomAFrench TomAFrench enabled auto-merge February 28, 2025 11:24
@TomAFrench TomAFrench added this pull request to the merge queue Feb 28, 2025
@aakoshh
Copy link
Contributor Author

aakoshh commented Feb 28, 2025

we're building up a number of redundant versions of ProgramArtifact

Which one do you mean? Getting rid of redundancy was my motivation for getting rid of the Circuit in this crate and forcing things back to be CompiledProgram.

@TomAFrench
Copy link
Member

Sorry, my comment isn't specific to this PR. Just something I'm thinking about related to the CLI in general recently, I think we're on the same page.

Merged via the queue into master with commit ebaff44 Feb 28, 2025
102 checks passed
@TomAFrench TomAFrench deleted the 7380-nargo-exec-fwd branch February 28, 2025 11:45
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
2 participants