Unify configuration structure for Lurk library and CLI #706

samuelburnham · 2023-09-26T16:07:55Z

Moves the LURK_DIRS global variable to config.rs as LURK_CONFIG: OnceCell<Settings>. This enables the configuration of the public parameter cache location in the following ways:
- Lurk as a library:
  - Add public_params_dir = "/path/to/public_params" into the config file located at $HOME/.lurk/lurk.toml or a configurable location (lowest priority)
  - Set the LURK_PUBLIC_PARAMS_DIR env var (higher priority)
- Lurk CLI:
  - Pass the --public-params-dir /path/to/public_params arg to the lurk binary to override the env var and config file (highest priority).
The CLI mirrors the above settings in its own CLI_CONFIG: OnceCell<CliSettings>, while sharing the lurk.toml config file for now:
- Config file key: proofs_dir, commits_dir, circom_dir
- Env var: LURK_PROOFS_DIR, LURK_COMMITS_DIR, LURK_CIRCOM_DIR
- CLI args: --proofs-dir, --commits-dir, --circom-dir
Removes the cache path arg from the public_params function in favor of the new config
Refactors the CLI's set_lurk_dirs() function to lurk_config() and cli_config(), which use the OnceCell::get_or_init() function to remove the prior reliance on function call order.
Disambiguate/merge configs with the preexisting src/config.rs concerning parallelism. If merged, the configs would all be contained in $HOME/.lurk/lurk.toml with the source in src/config.rs
Add doc comments, particularly regarding the config options explained above

Fixes #621

huitseeker · 2023-09-27T15:42:23Z

Note, even if I haven't had the time to give this a review: @samuelburnham and @winston-h-zhang have some overlap between this and #648

src/public_parameters/disk_cache.rs

winston-h-zhang · 2023-10-09T19:05:19Z

@samuelburnham what is the status of this PR? I agree with your comment that the Lurk config should be merged into config.rs. Otherwise, I think this PR looks good and we should stabilize these changes. I think it's likely that more changes will be required as the needs of devex grows, but we can deal with that as it comes.

arthurpaulino

I like the general direction this is heading towards. Tyvm!

I wonder if the default config file should live in .config or .lurk. I usually like it when applications populate a single folder on my machine by default, but that's a subjective taste.

src/cli/field.rs

arthurpaulino

Some streamlined comments about the utils crate. I believe it's too simple to deserve its own crate right now. Especially because everything here seems to have a better home

utils/src/lib.rs

arthurpaulino

More comments from a third pass, now focusing on nitty gritty details

src/public_parameters/disk_cache.rs

src/config.rs

src/cli/mod.rs

src/cli/config.rs

winston-h-zhang

I think this PR is headed in the right direction. There are a few remaining complexities I'd like to iron out:

Preferably, I would like everything to be in one global config (merging CLI_CONFIG into config::LURK_CONFIG here), although I'm not strongly attached to the idea. Another reason is that we should be against the CLI having "ownership" over any part of the config (see pq's comment in the notion).
In the same way the CLI should not have any privilege to any particular config, the CLI should not have any privilege to modify environment variables. As we currently have it passing CLI arguments works by first overwriting the env vars of the corresponding arguments, and then propagating that change up to the config. This is really unpredictable; you might have default env vars, then accidentally overwrite everything by copy pasting someone else's CLI command.
Documentation in config.rs detailing the behavior of the config. Is the config.toml initialized to default values or does the user have to create one manually? Does the config.toml stay in sync with the env vars, and/or are there any other automatic changes done which the user should be aware of? If I want to permanently change the default public params directory, do I have to manually edit public_params_default_dir(), is it safe to do so, etc?

Addressing each of these points:

Merge CLI_CONFIG into config::LURK_CONFIG.
We should find some way to directly mutate the config from the CLI without taking a detour through env vars. Naively, I just want to make this a mutable static global, but @huitseeker definitely knows the better conventions here.
I think these questions are more iffy and could warrant more discussion, so any ideas are welcome!

src/cli/config.rs

src/config.rs

Cargo.toml

benches/common/mod.rs

src/cli/config.rs

arthurpaulino

This is amazing work 🙏🏼

Earlier in the (short) call we had you mentioned that the user would be able to use a custom config file but I don't see this signaled in the CLI help menu nor anywhere else for the user to know. How do you think the user should become aware of that feature? Or is it not implemented yet?

benches/common/mod.rs

samuelburnham · 2023-10-24T00:10:33Z

@arthurpaulino the config file itself can't be customized, but its location can be configured by the --config CLI arg that already exists, or by the methods documented here: https://github.com/lurk-lab/lurk-rs/blob/ef9ebd6e5bf2fa268f5992f3920ad8dd82ea0eb6/src/config.rs#L42

arthurpaulino

I tested it and it seems to be fine. Maybe others can test it too

huitseeker

This looks good to me globally, with some comments left inline.

src/config.rs

benches/common/mod.rs

src/cli/config.rs

Unify public param config with config.rs, spin out CLI config Address feedback and refactor CLI overrides Simplify global initialization Add configurable config file location

huitseeker

Thanks a ton!

samuelburnham force-pushed the config-public-params branch from 3512e79 to 6e7ccb1 Compare September 26, 2023 20:54

samuelburnham marked this pull request as ready for review September 26, 2023 20:55

samuelburnham requested review from a team as code owners September 26, 2023 20:55

samuelburnham force-pushed the config-public-params branch from 6e7ccb1 to 0ca1996 Compare September 26, 2023 21:01

arthurpaulino reviewed Sep 27, 2023

View reviewed changes

src/public_parameters/disk_cache.rs Outdated Show resolved Hide resolved

samuelburnham force-pushed the config-public-params branch 2 times, most recently from e7579c6 to feb4f03 Compare October 13, 2023 23:52

arthurpaulino reviewed Oct 14, 2023

View reviewed changes

src/cli/field.rs Outdated Show resolved Hide resolved

arthurpaulino reviewed Oct 14, 2023

View reviewed changes

utils/src/lib.rs Outdated Show resolved Hide resolved

utils/src/lib.rs Outdated Show resolved Hide resolved

arthurpaulino reviewed Oct 14, 2023

View reviewed changes

src/public_parameters/disk_cache.rs Outdated Show resolved Hide resolved

src/config.rs Outdated Show resolved Hide resolved

src/cli/mod.rs Show resolved Hide resolved

src/cli/config.rs Outdated Show resolved Hide resolved

winston-h-zhang reviewed Oct 16, 2023

View reviewed changes

src/cli/config.rs Show resolved Hide resolved

src/config.rs Outdated Show resolved Hide resolved

huitseeker reviewed Oct 17, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

samuelburnham force-pushed the config-public-params branch 2 times, most recently from 88fe620 to df030a9 Compare October 18, 2023 17:58

samuelburnham requested review from huitseeker, winston-h-zhang, arthurpaulino and porcuquine October 18, 2023 18:04

samuelburnham force-pushed the config-public-params branch from 2a0e685 to 54f6540 Compare October 19, 2023 14:24

samuelburnham commented Oct 19, 2023

View reviewed changes

benches/common/mod.rs Outdated Show resolved Hide resolved

samuelburnham mentioned this pull request Oct 21, 2023

end2end_lem bench uses uncached public params #778

Closed

samuelburnham force-pushed the config-public-params branch from 54f6540 to ef9ebd6 Compare October 23, 2023 17:32

samuelburnham requested a review from a team as a code owner October 23, 2023 17:32

samuelburnham commented Oct 23, 2023

View reviewed changes

src/cli/config.rs Show resolved Hide resolved

arthurpaulino reviewed Oct 23, 2023

View reviewed changes

benches/common/mod.rs Outdated Show resolved Hide resolved

arthurpaulino previously approved these changes Oct 24, 2023

View reviewed changes

samuelburnham changed the title ~~fix: Move public param cache config to src/public_parameters/disk_cache~~ Unify configuration structure for Lurk library and CLI Oct 24, 2023

samuelburnham dismissed arthurpaulino’s stale review via b406765 October 24, 2023 03:15

samuelburnham force-pushed the config-public-params branch from ef9ebd6 to b406765 Compare October 24, 2023 03:15

samuelburnham linked an issue Oct 24, 2023 that may be closed by this pull request

end2end_lem bench uses uncached public params #778

Closed

huitseeker reviewed Oct 24, 2023

View reviewed changes

src/config.rs Show resolved Hide resolved

benches/common/mod.rs Outdated Show resolved Hide resolved

src/cli/config.rs Show resolved Hide resolved

Refactor public param config

962f220

Unify public param config with config.rs, spin out CLI config Address feedback and refactor CLI overrides Simplify global initialization Add configurable config file location

samuelburnham force-pushed the config-public-params branch from b406765 to 962f220 Compare October 24, 2023 21:08

samuelburnham mentioned this pull request Oct 24, 2023

[Post-#706] Improve TOML config file UX #793

Open

arthurpaulino approved these changes Oct 24, 2023

View reviewed changes

samuelburnham mentioned this pull request Oct 24, 2023

[Post-#706] Integrate CLI config options more directly with clap #794

Open

samuelburnham requested a review from huitseeker October 24, 2023 22:13

winston-h-zhang approved these changes Oct 24, 2023

View reviewed changes

huitseeker approved these changes Oct 24, 2023

View reviewed changes

samuelburnham added this pull request to the merge queue Oct 25, 2023

Merged via the queue into lurk-lab:master with commit aa67bdb Oct 25, 2023
10 of 12 checks passed

samuelburnham deleted the config-public-params branch October 25, 2023 00:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify configuration structure for Lurk library and CLI #706

Unify configuration structure for Lurk library and CLI #706

samuelburnham commented Sep 26, 2023 •

edited

Loading

huitseeker commented Sep 27, 2023

winston-h-zhang commented Oct 9, 2023

arthurpaulino left a comment

arthurpaulino left a comment •

edited

Loading

arthurpaulino left a comment

winston-h-zhang left a comment •

edited

Loading

arthurpaulino left a comment

samuelburnham commented Oct 24, 2023

arthurpaulino left a comment

huitseeker left a comment

huitseeker left a comment

Unify configuration structure for Lurk library and CLI #706

Unify configuration structure for Lurk library and CLI #706

Conversation

samuelburnham commented Sep 26, 2023 • edited Loading

huitseeker commented Sep 27, 2023

winston-h-zhang commented Oct 9, 2023

arthurpaulino left a comment

Choose a reason for hiding this comment

arthurpaulino left a comment • edited Loading

Choose a reason for hiding this comment

arthurpaulino left a comment

Choose a reason for hiding this comment

winston-h-zhang left a comment • edited Loading

Choose a reason for hiding this comment

arthurpaulino left a comment

Choose a reason for hiding this comment

samuelburnham commented Oct 24, 2023

arthurpaulino left a comment

Choose a reason for hiding this comment

huitseeker left a comment

Choose a reason for hiding this comment

huitseeker left a comment

Choose a reason for hiding this comment

samuelburnham commented Sep 26, 2023 •

edited

Loading

arthurpaulino left a comment •

edited

Loading

winston-h-zhang left a comment •

edited

Loading