-
Notifications
You must be signed in to change notification settings - Fork 50
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
Move the experimental frontends into a separate crate, so that when not using them they don't take several minutes to compile (and indirect dependencies). #168
Conversation
9bdacb5
to
27ab0e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please accept apologies for interrupting, but have you considered hiding each frontend behind a dedicated feature (and making deps optional)?
Edit: I see you have :) can you share the reasons to abandon features in favour of crates?
Indeed, that's a good point. This was initially done at #141 but the issue is that even if a frontend was 'not used' by not using its feature, it's dependencies were still being imported (and the dependencies of the dependencies, taking long minutes and MBs of data), when running tests but also when using Sonobe as external dependency. With the separation into a different subpackage for the experimental frontends done in this PR, the users of the lib that don't use any of the experimental frontends will not get impacted by those dependencies that are in fact not used in that case, and only in the cases where the experimental frontends are specifically used those dependencies will get used. As a side note, the recommended frontend is using arkworks (directly implementing the FCircuit trait), as the other experimental frontends add overhead when generating the witness which makes the folding step slower. |
oh, okay; now I remember similar issue I had with private repo dependency in a workspace causing problems for every member crate, even for those, that didn't use it (even indirectly); afair a workaround it was to use some dummy patching, but in Sonobe case I totally understand and support your final decision - thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
2bb66c1
to
b42999f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion regarding the README.md
of the frontend crate. Otherwise lgtm!
b42999f
to
bff2c55
Compare
Let's just merge this @arnaucube as I want to wait for Janabel to confirm that the PR works prev to merge it. |
92b744e
to
fe3f84a
Compare
c902429
to
0a9526d
Compare
49a871a
to
8f01d87
Compare
…ot using them they don't take several minutes to compile (and indirect dependencies). This saves several minutes (and MBs of data) on compilation time both when running tests in this repo, but also when using the sonobe lib as a dependency in external repos.
8f01d87
to
43aa99e
Compare
Move the experimental frontends into a separate crate, so that when not using them they don't take several minutes to compile (and indirect dependencies).
This saves several minutes (and MBs of data) on compilation time both when running tests in this repo, but also when using the sonobe lib as a dependency in external repos.
This solution was suggested by @ed255 when talking about this.