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.

Sign up for GitHub

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

Jump to bottom

Circuit cache dir now uses os cache dir #405

Merged
merged 5 commits into from
Aug 12, 2024
Merged

Circuit cache dir now uses os cache dir #405

merged 5 commits into from
Aug 12, 2024

Conversation

BGluth
Copy link
Contributor

@BGluth BGluth commented Jul 17, 2024

Resolves #375.

  • Now attempts to use the OS's specific cache dir to store circuits.
  • This is done by setting the env var ZK_EVM_CACHE_DIR if the user has not already set it, which allows the user to override the circuit cache directory if they want to.

@BGluth BGluth requested a review from 0xaatif July 17, 2024 20:16
@BGluth BGluth requested review from muursh and Nashtare as code owners July 17, 2024 20:16
@github-actions github-actions bot added the crate: zero_bin Anything related to the zero-bin subcrates. label Jul 17, 2024
BGluth
BGluth commented Jul 17, 2024
View reviewed changes
zero_bin/common/src/prover_state/mod.rs Outdated
@@ -242,6 +243,7 @@ impl ProverStateManager {
/// Initialize global prover state from the configuration.
pub fn initialize(&self) -> anyhow::Result<()> {
info!("initializing prover state...");
set_circuit_cache_dir_env_if_not_set()?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both the leader & worker need to persist circuits, so I thought it made sense to do the env logic inside common. Let me know if anyone prefers this to be in the individual binaries.

zero_bin/common/src/prover_state/persistence.rs Outdated Show resolved Hide resolved
@BGluth
Copy link
Contributor Author

BGluth commented Jul 17, 2024

Also, do we want a clean command to clear out any cached circuits now that we're writing a potentially large amount of data to a OS specific cache (which the user might not be aware is happening)?

@Nashtare Nashtare added this to the System strengthening milestone Jul 17, 2024
BGluth added a commit that referenced this pull request Jul 18, 2024
@BGluth
Requested PR changes for #405
e79d82c 
- No longer uses a lazy static.
BGluth added a commit that referenced this pull request Jul 18, 2024
@BGluth
Requested PR changes for #405
8c10257 
- No longer uses a lazy static.
@BGluth BGluth force-pushed the feat/circuit_cache_dir_os_specific branch from e79d82c to 8c10257 Compare July 18, 2024 17:25
BGluth added a commit that referenced this pull request Jul 18, 2024
@BGluth
Requested PR changes for #405
993d7e2 
- No longer uses a lazy static.
@BGluth BGluth force-pushed the feat/circuit_cache_dir_os_specific branch from 8c10257 to 993d7e2 Compare July 18, 2024 17:52
BGluth added a commit that referenced this pull request Jul 18, 2024
@BGluth
Requested PR changes for #405
1fea504 
- No longer uses a lazy static.
@BGluth BGluth force-pushed the feat/circuit_cache_dir_os_specific branch from 993d7e2 to 1fea504 Compare July 18, 2024 17:58
@BGluth
Copy link
Contributor Author

BGluth commented Jul 30, 2024

Regarding naming, this is already implied by the hardcoded constants + doc that we intend to specify the dir env at ZK_EVM_CACHE_DIR_ENV, so I would simplify the name of the method to set_circuit_cache_dir_env

@Nashtare I swear I responded to this last week, not sure what happened...

So just to be clear, the env var set_circuit_cache_dir_env_if_not_set is able to be set by the user manually, and if they set the env var, this function returns early and does not set it. I feel if we rename this to set_circuit_cache_dir_env then it might be a bit misleading just because it really won't set anything if it's already set. I might be misunderstanding, but let me know what you think.

BGluth added a commit that referenced this pull request Aug 1, 2024
@BGluth
Requested PR changes for #405
0381d92 
- No longer uses a lazy static.
BGluth added a commit that referenced this pull request Aug 1, 2024
@BGluth
Requested PR changes for #405 (2)
70cb4db 
- Fixed accidently not panicing when an `env` var is not set.
@BGluth BGluth force-pushed the feat/circuit_cache_dir_os_specific branch from 1fea504 to 70cb4db Compare August 1, 2024 18:50
Nashtare
Nashtare approved these changes Aug 9, 2024
View reviewed changes
Copy link
Collaborator

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

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

I'm good with this overall

BGluth added 3 commits August 9, 2024 12:23
@BGluth
Circuit cache dir now uses os cache dir (#375)
09160c1 
- Now attempts to use the OS's specific cache dir to store circuits.
- This is done by setting the env var `ZK_EVM_CACHE_DIR` if the user has
  not already set it, which allows the user to override the circuit
  cache directory if they want to.
@BGluth
Requested PR changes for #405
cfa25c5 
- No longer uses a lazy static.
@BGluth
Requested PR changes for #405 (2)
f6200c2 
- Fixed accidently not panicing when an `env` var is not set.
@BGluth BGluth force-pushed the feat/circuit_cache_dir_os_specific branch from 70cb4db to f6200c2 Compare August 9, 2024 18:23
@BGluth BGluth mentioned this pull request Aug 9, 2024
Merged
@BGluth BGluth enabled auto-merge (squash) August 12, 2024 22:57
@BGluth BGluth merged commit 1a54af9 into develop Aug 12, 2024
15 checks passed
@BGluth BGluth deleted the feat/circuit_cache_dir_os_specific branch August 12, 2024 23:12
atanmarko pushed a commit that referenced this pull request Aug 14, 2024
@BGluth @atanmarko
Circuit cache dir now uses os cache dir (#405)
abd4ceb 
* Circuit cache dir now uses os cache dir (#375)

- Now attempts to use the OS's specific cache dir to store circuits.
- This is done by setting the env var `ZK_EVM_CACHE_DIR` if the user has
  not already set it, which allows the user to override the circuit
  cache directory if they want to.

* Requested PR changes for #405

- No longer uses a lazy static.

* Requested PR changes for #405 (2)

- Fixed accidently not panicing when an `env` var is not set.

* Fixed env var not being set in AMPQ mode

* Requested PR changes for #405 (3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xaatif 0xaatif 0xaatif left review comments

@Nashtare Nashtare Nashtare approved these changes

@muursh muursh Awaiting requested review from muursh

Assignees

@BGluth BGluth

Labels
crate: zero_bin Anything related to the zero-bin subcrates.
Projects
Zero EVM
Status: Done
Milestone
System strengthening
Development

Successfully merging this pull request may close these issues.

Move circuit directory to common platform dependant directory
3 participants
@BGluth @Nashtare @0xaatif