feat!: Log Directive support for traces #105

vezenovm · 2023-02-17T22:03:51Z

…from printing when solving directives

Related issue(s)

Resolves #95

Description

Directive::Log no longer prints the evaluated log while it is being evaluating during partial witness generation. This enables us to have both trace functionality and move logic for displaying logs to the code that calls the PWG rather than keeping it with solver itself.

Summary of changes

solve under the PWG now takes in a vector of directives. These directives are limited to be Directive::Log as it does not make sense to be returning any other directive. The log directive that is returned must have have finalized output or else that means there is a critical bug.

Several more changes showing how to handle logs correctly will be supplied in nargo.

Dependency additions / changes

(If applicable.)

Test additions / changes

(If applicable.)

Checklist

I have tested the changes locally.
I have formatted the changes with Prettier and/or cargo fmt with default settings.
I have linked this PR to the issue(s) that it resolves.
I have reviewed the changes on GitHub, line by line.
I have ensured all changes are covered in the description.

Additional context

(If applicable.)

…from printing when solving directives

acir/src/circuit/opcodes.rs

acir/src/circuit/directives.rs

acir/src/circuit/opcodes.rs

guipublic

It looks like you need to persist some state during the solve. I also needed this for solving the new block opcode on my PR #114. The solve is no more recursive and we can instantiate data that persist throughout the solve process without adding specific parameters and return values. I suggest you branch from it.

acvm/src/pwg/directives.rs

TomAFrench

I'm happy with this PR now. @guipublic Can you remove the block as we've addressed the recursive solve function?

guipublic

My two comments are to remove the logging logic from ACIR and to keep the solving coherent with the other gates.

guipublic · 2023-02-23T09:35:53Z

acir/src/circuit/directives.rs

@@ -58,7 +58,10 @@ pub enum Directive {
        bits: Vec<Witness>, // control bits of the network which permutes the inputs into its sorted version
        sort_by: Vec<u32>, // specify primary index to sort by, then the secondary,... For instance, if tuple is 2 and sort_by is [1,0], then a=[(a0,b0),..] is sorted by bi and then ai.
    },
-    Log(LogInfo),
+    Log {
+        is_trace: bool, // This field states whether the log should be further manipulated or simply displayed to standard output


Why do we have two separate process, and not just one which basically notifies when the 'log witness' are solved? Then the caller can decide to print to standard output or to further manipulate them. This logic should not be in ACIR.

This is needed as without this flag the caller does not know whether to print to standard output or to further manipulate the logs. I was trying to have the same directive for the builtins std::println and std::trace. For example, if solve simply returns a finalized string to be logged, but both std::println and std::trace exist, the caller will not know whether to print to std out or manipulate the trace how it sees fit without more info. The flag would be set during the evaluator, and essentially passed along until solve is called. Only the caller of solve would implement the logic of how to handle a log according to the flag.

I am not suggesting to remove the flag, sorry for the mis-understanding, the location of my comment is confusing. This is because my first reaction was to ask for a clarification on the flag meaning. But at the end, I think it is better to not handle it in the solve, and just solve the witness.
With the flag (from the directive), the caller should be able to handle it correctly, shouldn't he?

Yes that is the case. We only pass the flag along with the solved witness info so that the caller can determine how they should handle the solved log directive. The process in solve_directives for Directive::Log remains the same for both traces and printlns.

The flag is also used during the solve, that's why I thought we were implementing its logic.
The solve is handling the flag and also behaves differently when there is only one witness. Could you explain why we have these 2 special cases?

So currently we only support logs of individual fields, arrays, or strings (formatting and handling of complex types can come in a separate PR). In the case of one witness we don't have to construct a list of hex elements. We can simply format the respective witness value and return that the log has been solved. In the case of multiple witnesses this means we have an array, where we have to fetch a list of hex elements, and format them into one finalized string output. Both cases will form a LogOutputInfo::FinalizedOutput(String), which the caller of solve will then handle. In the case of a string this is formatted during acir gen and should already be a LogOutputInfo::FinalizedOutput. If Directive::Log already has finalized output we can just return the string stored in the enum.

Thank you for the explanation. For me you are processing the logs here and you should not. The Log directive should just hold the parameters (for now just the flag), and the witness to log.
When the gate is solved, i.e when all the witness are known, you can send the parameters and witness values to the caller. That's it. How/where to print, which language to use, etc... all this should be handled by the caller.

Got it, I see now. Ok I will refactor this

guipublic · 2023-02-23T09:38:34Z

acvm/src/pwg/directives.rs

@@ -15,13 +15,13 @@ use super::{get_value, insert_value, sorting::route, witness_to_value};
 pub fn solve_directives(
    initial_witness: &mut BTreeMap<Witness, FieldElement>,
    directive: &Directive,
-) -> Result<(), OpcodeResolutionError> {
+) -> Result<Option<Directive>, OpcodeResolutionError> {


I don't think the solve should return an optional directive. The directive is either solved or not. If it is solved, then you can get the witness values from the witness map.

Agreed - If a directive is not solved, then it means we did not find a witness that it needed, this will return an Error

I was unsure on whether I should do this but I went with returning a directive mainly to remove duplication of code as the logging information needed by whomever is calling solve is very similar to the Log directive itself. Whomever is calling solve must check that we have a finalized string output and whether the log is a trace or a println. This can be seen in this draft PR: noir-lang/noir#872. I will look into changing this as I agree it confuses the idea of whether the directive has been solved or not.

kevaundray · 2023-02-23T11:50:22Z

It looks like you need to persist some state during the solve. I also needed this for solving the new block opcode on my PR #114. The solve is no more recursive and we can instantiate data that persist throughout the solve process without adding specific parameters and return values. I suggest you branch from it.

This should not have been a blocking change to this PR. In general, we should not branch off of or copy unrelated PRs. If we want to remove recursion, so that two separate PRs can benefit from it, we should have a PR that does this, merge it into master and then have both PRs rebase.

This is beneficial for two reasons:

Avoids the situation where two PRs are doing the same thing on two separate branches which will show a diff for the reviewer on both PRs until one of them is merged
Doesn't block the core of the original PRs which in most cases is unrelated

vezenovm · 2023-03-07T23:09:13Z

Closing in favor of PR #137

initial commit changing log directive to accept traces and move away …

980e2cc

…from printing when solving directives

vezenovm changed the title ~~Log Directive support for traces~~ feat!: Log Directive support for traces Feb 17, 2023

This was referenced Feb 17, 2023

feat!: Incorporate Log Directive traces update in ACVM [DO NOT MERGE] noir-lang/acvm-backend-barretenberg#57

Closed

feat(std_lib): Support extracting intermediate witnesses from a circuit using traces noir-lang/noir#872

Closed

jfecher reviewed Feb 21, 2023

View reviewed changes

acir/src/circuit/opcodes.rs Show resolved Hide resolved

acir/src/circuit/directives.rs Outdated Show resolved Hide resolved

Merge branch 'master' into mv/trace

eddc64a

TomAFrench reviewed Feb 22, 2023

View reviewed changes

acir/src/circuit/opcodes.rs Show resolved Hide resolved

acir/src/circuit/opcodes.rs Show resolved Hide resolved

guipublic suggested changes Feb 22, 2023

View reviewed changes

TomAFrench reviewed Feb 22, 2023

View reviewed changes

acvm/src/pwg/directives.rs Outdated Show resolved Hide resolved

TomAFrench mentioned this pull request Feb 22, 2023

chore(acvm): switch to non-recursive solve method #115

Merged

5 tasks

TomAFrench added 3 commits February 22, 2023 18:49

chore: add comment on the assumption in printing witnesses

f7b98fa

chore: remove dbg! call

a7227ee

Merge branch 'master' into mv/trace

def272e

TomAFrench approved these changes Feb 22, 2023

View reviewed changes

guipublic suggested changes Feb 23, 2023

View reviewed changes

vezenovm closed this Mar 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Log Directive support for traces #105

feat!: Log Directive support for traces #105

vezenovm commented Feb 17, 2023

guipublic left a comment

TomAFrench left a comment

guipublic left a comment

guipublic Feb 23, 2023

vezenovm Feb 23, 2023

guipublic Feb 23, 2023 •

edited

Loading

vezenovm Feb 23, 2023

guipublic Feb 24, 2023

vezenovm Feb 24, 2023

guipublic Feb 24, 2023

vezenovm Feb 24, 2023

guipublic Feb 23, 2023

kevaundray Feb 23, 2023

vezenovm Feb 23, 2023 •

edited

Loading

kevaundray commented Feb 23, 2023

vezenovm commented Mar 7, 2023

feat!: Log Directive support for traces #105

feat!: Log Directive support for traces #105

Conversation

vezenovm commented Feb 17, 2023

Related issue(s)

Description

Summary of changes

Dependency additions / changes

Test additions / changes

Checklist

Additional context

guipublic left a comment

Choose a reason for hiding this comment

TomAFrench left a comment

Choose a reason for hiding this comment

guipublic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guipublic Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vezenovm Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

kevaundray commented Feb 23, 2023

vezenovm commented Mar 7, 2023

guipublic Feb 23, 2023 •

edited

Loading

vezenovm Feb 23, 2023 •

edited

Loading