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

Default pipeline with backend arguments #2557

Open
wants to merge 3 commits into
base: pilopt-two-pass
Choose a base branch
from

Conversation

qwang98
Copy link
Collaborator

@qwang98 qwang98 commented Mar 18, 2025

Depends on #2535 and standardizes the backend argument in Pipeline:

  1. backend now defaults to BackendType::Mock when constructing the pipeline.
  2. For tests that only computes up to witgen without actually running the backend, with_backend_factory API is used instead of with_backend.
  • If the test doesn't care which backend the witgen is computed for, NO with_backend_factory is called, because the default pipeline already defaults to BackendType::Mock.
  • Note that with_backend_factory(backend) is equivalent to with_backend(backend, None).
  1. For tests that generate proof, with_backend API is used.

@qwang98 qwang98 changed the base branch from main to pilopt-two-pass March 18, 2025 06:57
@qwang98 qwang98 requested a review from Schaeff March 18, 2025 11:36
@qwang98 qwang98 marked this pull request as ready for review March 18, 2025 11:36
Copy link
Collaborator

@Schaeff Schaeff left a comment

Choose a reason for hiding this comment

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

Looks good, just wondering if we could have with_backend_factory and with_backend_options instead of the overlapping methods now. This is not block though.

@@ -97,14 +97,14 @@ impl<R: io::Read> AsIoRead for Option<R> {
}

/// Optional Arguments for various stages of the pipeline.
#[derive(Default, Clone)]
#[derive(Clone)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not implement default on backend and keep this?

@@ -127,6 +127,29 @@ struct Arguments<T: FieldElement> {
existing_proof_file: Option<PathBuf>,
}

impl<T> Default for Arguments<T>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would then go away

Depends on #2535 and #2557. Split
`assert_proofs_fail_for_invalid_witnesses_pilcom` into
`assert_proofs_fail_for_invalid_witnesses_pilcom_composite` and
`assert_proofs_fail_for_invalid_witnesses_pilcom_monolithic`. These two
APIs are needed because we might generate different witnesses for pilcom
composite vs pilcom monolithic under two scenarios:
1. The two backends can have different witnesses to start with. This
hasn't been an issue because most of our test cases are very simple
machines.
2. After updates in #2535, if we eventually implement different
optimizations for the two different backends, they might compute two
different witnesses.
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.

2 participants