chore: Bootstrap cache download ignores PARALLELISM #11030
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We had introduced the PARALLELISM env var for bootstrap in #10909 so we could build on non-mainframe machines. However, setting a low parallelism means that requests to the cache when building noir circuits happen serially, so latency now becomes a problem. This PR is an attempt to set up a pipeline of parallel operations, where we first run all cache downloads with very high parallelism, output what needs to be built, and run that with a lower parallelism to avoid killing the host.
spalladino
commented
Jan 3, 2025
| function build { | ||
| set +e | ||
| set -u | ||
|
|
||
| [ -f "package.json" ] && denoise "yarn && node ./scripts/generate_variants.js" | ||
| local parallel_cmd="parallel -t --line-buffer --halt now,fail=1" |
There was a problem hiding this comment.
I had to remove --tag since it polluted stdout. With --tag set, the input to the second command (compile_circuit) included the tag. However, by removing the tag we now don't get any feedback on what circuit are we running.
Any idea on how to make --tag only apply to stderr? Or should I arrange this in a different way? An easy solution is to manually include the circuit name in every echo_stderr we do here, since there are not too many. But I'd like to know what's the proper way of handling this.
spalladino
added a commit
that referenced
this pull request
Jan 3, 2025
This PR removes the PARALLELISM for building each contract and circuit that had been introduced in #10909. Instead, it adds a `sem` around the resource-hungry sections of code, using the PARALLELISM default. This allows for more parallelism of the entire script, while slowing down only on the parts where it's really needed. Alternative to #11030.
spalladino
added a commit
that referenced
this pull request
Jan 6, 2025
This PR removes the PARALLELISM for building each contract and circuit that had been introduced in #10909. Instead, it adds a `sem` around the resource-hungry sections of code, using the PARALLELISM default. This allows for more parallelism of the entire script, while slowing down only on the parts where it's really needed. Alternative to #11030.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We had introduced the PARALLELISM env var for bootstrap in #10909 so we could build on non-mainframe machines. However, setting a low parallelism means that requests to the cache when building noir circuits happen serially, so latency now becomes a problem.
This PR is an attempt to set up a pipeline of parallel operations, where we first run all cache downloads with very high parallelism, output what needs to be built, and run that with a lower parallelism to avoid killing the host.