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

Abstract over Run_state, include it in the snarky backend #858

Merged
merged 5 commits into from
Jan 7, 2025

Conversation

mrmr1993
Copy link
Member

@mrmr1993 mrmr1993 commented Jan 6, 2025

This PR abstracts over Run_state, accepting it as a parameter from the snarky backend. This allows us to pass through a rust implementation in place of the default OCaml implementation.

@@ -0,0 +1,89 @@
module Vector : sig
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if making an external library for vectors (and the other types we use in Pickles) would make sense. Could be nice for the OCaml community too.

Copy link
Member

Choose a reason for hiding this comment

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

Not blocking, but I don't understand why we have the nested module Vector with the methods, and externally we do have only the type, both types being equal.

@@ -341,12 +349,14 @@ struct
Checked_intf.Basic
with module Types := Types
with type field := Checked_runner.field
and type run_state := run_state
Copy link
Member

Choose a reason for hiding this comment

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

Not blocking: wondering if we could not remove some type aliases above to simplify the interface. type state = run_state for instance. Only looking at the lines above, it might not make sense as used for something else.

@@ -215,7 +217,7 @@ struct
Runner.State.make ~num_inputs ~input ~next_auxiliary ~aux ~system
~with_witness:false ()
in
let id = Run_state.id state in
let id = Backend.Run_state.id state in
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this doesn't seem to be useful (compiles without on my side), as the module is opened L21. Or, are you trying to remove the statements open 🥹?

@@ -714,7 +715,7 @@ module Run = struct

module Make_basic (Backend : Backend_intf.S) = struct
module Snark = Make (Backend)
open Run_state
open Backend.Run_state
Copy link
Member

Choose a reason for hiding this comment

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

It seems we can remove this open by changing L1564. See 242d5b5.

Copy link
Member

Choose a reason for hiding this comment

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

Suggesting as everywhere we use the full path.

Copy link
Member

@dannywillems dannywillems left a comment

Choose a reason for hiding this comment

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

Left a few comments. Interested in seeing where this goes :).

@dannywillems dannywillems merged commit b994dae into master Jan 7, 2025
3 checks passed
@mrmr1993 mrmr1993 deleted the feature/abstract-run-state branch January 7, 2025 12:14
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