feat(node): load args from file #494

GMKrieger · 2025-02-13T22:15:30Z

Pull Request type

Feature

What is the current behavior?

Currently, the node doesn't allow arguments to be loaded from a file.

Resolves: #285

What is the new behavior?

Tries to load arguments from a file if a path is provided on the cli
Loads data from cli after that

Does this introduce a breaking change?

No.

Other information

I used figment, as it was the crate that worked best, but I know there are others, mainly twelf and config. If someone has experience with them, please, let me know!

Trantorian1

A welcome change! This has been in the talks for a while but was never properly implemented, thanks a lot for this!

A few points still need clarifying / changing, see comments below.

Trantorian1 · 2025-02-14T07:41:24Z

README.md

+### Configuration files
+
+You can load the arguments directly from a file for ease of use.
+The supported file formats are `json`, `toml` and `yaml`.
+You can find examples on `configs/args`.


configs/args should be a hyperlink to the related folder on the repo.

Trantorian1 · 2025-02-14T07:46:17Z

crates/madara/node/src/cli/mod.rs

+    /// A path to a config file
+    #[clap(env = "MADARA_CONFIG_FILE", long, value_name = "NAME")]
+    pub config_file: Option<PathBuf>,


I'm not sure about the ergonomics of this. I think we should keep this here, but also try and auto-detect a config file in known locations (such as the current working directory), and then this option can be used for unusual configurations. Also this needs more CLI-facing documentation, such as which formats are allowed.

crates/madara/node/src/cli/mod.rs

Trantorian1 · 2025-02-14T07:54:21Z

crates/madara/node/src/main.rs

+    let cli_args = RunCmd::parse().apply_arg_preset();
+
+    // Put values from config file if they don't already exist.
+    if let Some(config_path) = cli_args.config_file.clone() {
+        config = match config_path.extension() {
+            None => bail!("Unsupported file type for config file."),
+            Some(os_str) => match os_str.to_str() {
+                Some("toml") => config.join(Toml::file(config_path)),
+                Some("json") => config.join(Json::file(config_path)),
+                Some("yaml") => config.join(Yaml::file(config_path)),
+                _ => bail!("Unsupported file type for config file."),
+            },
+        }
+    }


As mentioned before, I feel like we should first check for config files in some known location and only if it is not found then refer to the cli args to potentially load an arg file. It feels a little un-ergonomic if I have to specify a cli arg in order to load an arg file in order not to use cli args 🤪

Trantorian1 · 2025-02-14T07:57:28Z

It might be worth adding a "default" config file to the repo as well, so users can just compile madara and run it with cargo run only, wdyt?

GMKrieger · 2025-02-17T18:06:41Z

It might be worth adding a "default" config file to the repo as well, so users can just compile madara and run it with cargo run only, wdyt?

Good idea! I'll work on one right away.

GMKrieger · 2025-02-18T21:01:55Z

@Trantorian1 I have run into an issue when doing the changes you requested.

I thought that figment was correctly setting up the arguments in the priority defined in the issue, being cli > env > file. However, when setting up the default file, I realized I couldn't overwrite the values in no way. Searching online, I found the underlying issue and solution, but it's not flawless; it basically requires to have to identical structures, one with all options and one with definitive struct, which would make the whole cli very verbose. If you're curious, I found the information here.

I tried another config layering crate twelf, but the documentation is outdated, the examples with clap are incomplete, and overall the experience was very poor. From my tests, I gathered the problem is it doesn't really like nested structures. When trying to deserialize, it kept complaining about missing certain fields.

I tested clap-serde-derive, which is small and not very robust, and it also needs an additional structs and just didn't work as I expected, presenting several other problems I couldn't fix.

Finally, config-rs is one of the most downloaded and used crates dealing with layering of configs, but it doesn't explicitly support clap and examples are not clear on how I could merge both.

Any suggestions are deeply appreciated, as I don't see a clear and best way forward.

Cheers!

Trantorian1 · 2025-02-19T07:55:29Z

This is unfortunate :/ It's surprising that the ecosystem hasn't developed a crate with unified cli and file parsing yet. I guess this really just is an ergonomics problem, just having a config file is already a good starting point.

Just a thought, but could we not first check if a config file exists in ./config.toml for example, and if it does just deserialize it into the cli struct without running the arg parse step? IE: we only call clap::Parser::Parse if we cannot find a config file. Shouldn't this resolve the prioritization issue or am I missing something?

Anyway, thanks a lot for the deep investigation into this topic :)

Trantorian1 · 2025-02-19T07:57:46Z

(Also this is more of a nitpick but please avoid force-pushing after a review since this makes it impossible to view "changes since last review" 😅)

Trantorian1 · 2025-02-19T08:01:59Z

crates/madara/node/src/cli/mod.rs

@@ -134,9 +134,15 @@ impl ArgsPresetParams {
    ),
 )]
 pub struct RunCmd {
+    /// A path to a config file.
+    /// It builds the arguments from files, env and cli.


The second line feels a bit confusing in that it seems to depict arg parsing generally? Maybe we should just remove it and keep the last line.

GMKrieger · 2025-02-19T18:58:43Z

This is unfortunate :/ It's surprising that the ecosystem hasn't developed a crate with unified cli and file parsing yet. I guess this really just is an ergonomics problem, just having a config file is already a good starting point.

Just a thought, but could we not first check if a config file exists in ./config.toml for example, and if it does just deserialize it into the cli struct without running the arg parse step? IE: we only call clap::Parser::Parse if we cannot find a config file. Shouldn't this resolve the prioritization issue or am I missing something?

Anyway, thanks a lot for the deep investigation into this topic :)

The original idea is that if you had a file, but wanted to change one argument, you could do that through the cli; mix and match between different sources. Your solution works, but we make it exclusive, either we get all information from the file or through the cli.

I'm going do your suggestion, but a little different: if there's any argument on the cli, we use all of the cli. This is so we can have the default cargo run execution working without any input or needing to put the file in the correct spot.

So it'll be like this:

cargo run with no arguments uses a default config file present on configs/args
cargo run pointing to a file will use that file
cargo run with cli will ignore the files and use cli arguments

Trantorian1 · 2025-02-20T08:50:49Z

Okay got it, so we get file configuration but without CLI override. This makes sense.

HermanObst

@GMKrieger looking good!
I would love to add something that test this in CI.
Maybe we can replicate the JS E2E test (they take very little to run) and re-execute with this?

cc @Trantorian1

Trantorian1 · 2025-02-21T07:53:09Z

I would generally be in agreement. We have already had regressions in the past where certain argument configurations no longer worked as expected after a merge since there was no test on this. Then again, this also feels like adding scope onto this PR

HermanObst

Lets fix CI, the rest LGTM!

HermanObst

Actually Ill leave this with RFC until we fix CI
@GMKrieger

Trantorian1 requested review from Trantorian1, antiyro and jbcaron February 14, 2025 07:39

Trantorian1 requested changes Feb 14, 2025

View reviewed changes

Trantorian1 assigned GMKrieger Feb 14, 2025

Trantorian1 added sequencer Related to the sequencing logic and implementation node Related to the full node implementation labels Feb 14, 2025

GMKrieger force-pushed the feat/load-config-file branch from 709cd3b to 41983cf Compare February 18, 2025 19:25

Trantorian1 approved these changes Feb 19, 2025

View reviewed changes

GMKrieger marked this pull request as ready for review February 19, 2025 20:17

GMKrieger force-pushed the feat/load-config-file branch from 92678ab to e238c9b Compare February 20, 2025 14:15

HermanObst reviewed Feb 20, 2025

View reviewed changes

GMKrieger force-pushed the feat/load-config-file branch from aab9224 to 36ee253 Compare February 21, 2025 14:43

HermanObst approved these changes Feb 21, 2025

View reviewed changes

HermanObst requested changes Feb 21, 2025

View reviewed changes

GMKrieger force-pushed the feat/load-config-file branch 2 times, most recently from f69e27f to 03e5690 Compare February 25, 2025 13:56

GMKrieger added the Skip-Changelog Skip orchestrator changelog check label Feb 25, 2025

feat(node): load args from file

07d53f5

GMKrieger force-pushed the feat/load-config-file branch from 03e5690 to 07d53f5 Compare February 25, 2025 16:23

HermanObst approved these changes Feb 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(node): load args from file #494

feat(node): load args from file #494

GMKrieger commented Feb 13, 2025

Trantorian1 left a comment

Trantorian1 Feb 14, 2025

Trantorian1 Feb 14, 2025

Trantorian1 Feb 14, 2025

Trantorian1 commented Feb 14, 2025 •

edited

Loading

GMKrieger commented Feb 17, 2025

GMKrieger commented Feb 18, 2025

Trantorian1 commented Feb 19, 2025

Trantorian1 commented Feb 19, 2025 •

edited

Loading

Trantorian1 Feb 19, 2025

GMKrieger commented Feb 19, 2025

Trantorian1 commented Feb 20, 2025

HermanObst left a comment

Trantorian1 commented Feb 21, 2025 •

edited

Loading

HermanObst left a comment •

edited

Loading

HermanObst left a comment

feat(node): load args from file #494

Are you sure you want to change the base?

feat(node): load args from file #494

Conversation

GMKrieger commented Feb 13, 2025

Pull Request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

Trantorian1 left a comment

Choose a reason for hiding this comment

Trantorian1 Feb 14, 2025

Choose a reason for hiding this comment

Trantorian1 Feb 14, 2025

Choose a reason for hiding this comment

Trantorian1 Feb 14, 2025

Choose a reason for hiding this comment

Trantorian1 commented Feb 14, 2025 • edited Loading

GMKrieger commented Feb 17, 2025

GMKrieger commented Feb 18, 2025

Trantorian1 commented Feb 19, 2025

Trantorian1 commented Feb 19, 2025 • edited Loading

Trantorian1 Feb 19, 2025

Choose a reason for hiding this comment

GMKrieger commented Feb 19, 2025

Trantorian1 commented Feb 20, 2025

HermanObst left a comment

Choose a reason for hiding this comment

Trantorian1 commented Feb 21, 2025 • edited Loading

HermanObst left a comment • edited Loading

Choose a reason for hiding this comment

HermanObst left a comment

Choose a reason for hiding this comment

Trantorian1 commented Feb 14, 2025 •

edited

Loading

Trantorian1 commented Feb 19, 2025 •

edited

Loading

Trantorian1 commented Feb 21, 2025 •

edited

Loading

HermanObst left a comment •

edited

Loading