feat: add simulation config #166

enigbe · 2024-02-01T22:16:42Z

What this PR does

~~Adds a SimulationConfig struct that~~ Loads config values from an optional INI file and overwrites these values if corresponding command line arguments are passed
The config file contains custom values for the simulation

[simln.conf]
data_dir = "."
sim_file = "sim.json"
total_time=
print_batch_size = 500
log_level = "info"
expected_pmt_amt = 3800000
capacity_multiplier = 2
no_results = false
log_interval = 60

Makes configurable the log_interval after which results are logged

Related Issue(s)

Resolves Refactor: Create config struct for simulator #157

Update

This PR has changed to consider a standard way of handling configuration files as well as command line arguments. It currently introduces functionality to create and parse the Cli from an optionally-provided config file, to merge this Cli with that parsed from command line arguments, and to grant precedence to the options/values in the latter.
Secondly, this PR also replaces the logging crate from simple_logger to flexi_logger. flexi_logger allows dynamic update of log levels after initialization and was required to deal with the issue described here.
Finally, documentation was updated to reflect the introduction and use of a config file.

Note: There are some changes to unrelated files because of the requirement to wrap maximum width of code and comments at 120 column spaces. Thought to include them regardless following conversation from #164 on maintaining width.

carlaKC

Thanks for picking this up! Just did a first pass, only major commets about serialization / DRY-in up some of the config checks with a macro.

sim-cli/src/config.rs

carlaKC · 2024-02-02T16:53:40Z

sim-cli/src/config.rs

+        .context("Failed to deserialize configuration string into SimulationConfig")?;
+
+        // 2. Override config values with CLI arguments (if passed)
+        if let Some(data_directory) = data_dir {


This is quite a lot of code for similar functionality, can we do a macro to cut LOC?

I do agree too. I have written a macro for this. However the log statements are never printed to the terminal because we initialize the logger after loading and substituting configs.

macro_rules! cli_overwrite { ($config:expr, $cli:expr, $($field:ident),*) => { match (&mut $config, $cli) { (config_struct, cli_struct) => { $( if let Some(value) = cli_struct.$field { log::info!( "SimulatinConfig::{} {:?} overwritten by CLI argument {:?}", stringify!($field), config_struct.$field, value ); config_struct.$field = value; } )* } } }; }

I would like to replace the simple_logger to something that allows runtime update to log levels but not without agreement from others.

Ah that's annoying, happy for you to shop around for a different logger 👍

Thought we might be doing some more complex log-fu while we're running (which I didn't like the sound of), but if we're just going to set the level once off after we've parsed config works for me.

conf.json

okjodom · 2024-02-09T06:24:48Z

@enigbe here comes a late review!

I've read through the changes proposed and seem beyond the scope of #157

Add a SimulationConfig struct that passes all the configuration options to the simulator rather than individual params.

I was expecting we create this new struct in sim-lib and update Simulation::new() to take nodes, activity, and config as params. In sim-cli we can just add a type coercion which converts the cli params into SimulationConfig and use it after Cli.parse(). Example coercion below

impl Cli {
    fn to_simulation_config(&self) -> anyhow::Result<SimulationConfig> {
        Ok(SimulationConfig {
            total_time: self.total_time.unwrap_or(60),
            print_batch_size: self.print_batch_size,
            log_level: self.log_level,
            expected_pmt_amt: self.expected_pmt_amt,
            capacity_multiplier: self.capacity_multiplier,
            no_results: self.no_results,
        })
    }
}

async fn main() {
  let cli = Cli::parse();
  let opts = cli.to_simulation_config().unwrap();
  ...  
}

Any changes that require the .ini might make sense if there's a user scenario needing it. I'm not aware of any so deferring to @carlaKC

Make log_interval configurable while we're here (currently hard set to 60s)

cool beans on this part of the issue. change lgtm :)

carlaKC · 2024-02-12T14:24:27Z

While this is outside of the scope of the original issue, I think it's a really nice extension to simln / pretty standard way to handle configuration. I can see it being useful when using simln in larger deployments where you want to have a config file checked in rather than deal with cli flags, so my vote is to keep the proposed functionality 👍

I was expecting we create this new struct in sim-lib and update Simulation::new() to take nodes, activity, and config as params.

Same here - the original issue was opened because clippy gets unhappy with the number of parameters that Simulation:new takes, so we're going to want to add a struct to sim-lib.

carlaKC

Some nits + one major comment about having separate structs for CLI and config=] parsing and how we manage defaults. If possible, would like to squash those into a single struct (I think clap has some settings that would make that possible).

conf.ini

sim-cli/src/config.rs

carlaKC · 2024-02-12T16:11:04Z

conf.ini

+expected_pmt_amt = 3800000
+capacity_multiplier = 2
+no_results = false
+log_interval = 60


Once we've checked this file in, let's add it to .gitignore so that people can changes values without accidentally committing them?

sim-cli/src/config.rs

carlaKC · 2024-02-12T16:29:43Z

sim-lib/src/lib.rs

@@ -339,6 +339,8 @@ pub struct Simulation {
    activity_multiplier: f64,
    /// Configurations for printing results to CSV. Results are not written if this option is None.
    write_results: Option<WriteResults>,
+    /// Duration after which results are logged


nit: expand comment to explain that this is how often we log a summary of results

carlaKC · 2024-02-12T20:28:06Z

sim-cli/src/config.rs

+#[derive(Debug, Deserialize)]
+pub struct SimulationConfig {


Wondering whether we can move deserialization onto the existing cli struct and then just get CLI parsing to only override values that are present?

Then we can create a simple SimulationConfig struct in lib can just convert the cli -> config struct like @okjodom suggested.

carlaKC · 2024-02-12T20:30:47Z

sim-cli/src/main.rs

-pub const ACTIVITY_MULTIPLIER: f64 = 2.0;
-
-/// Default batch size to flush result data to disk
-const DEFAULT_PRINT_BATCH_SIZE: u32 = 500;


Not sure about moving these defaults from in the codebase to into a config file. That moves us from being able to run sim-ln easily on its own to always needing a config file (or lots of flags) which makes things less usable (thinking about server contexts like docker where we'd now need to copy this config file into the container).

I noted this in the changes I have introduced. The defaults remain, and allow us to handle the absence of a config file.

- loads config options from a file and overwrites with CLI args - adds configurable log interval -change configuration file type by doing the ffg: replacing conf.json with conf.ini, updating SimulationConfig::load(...) to read the new .ini file, implementing cli_overwrite!(...) macro to reduce LOC when overwriting config values with CLI arguments

- Implements a Cli::from_config_file(...) method to load configuration options from either a default or user-provided (via command line argument) config file - Merges the Cli-s generated from the config file and the parsed command line arguments taking into account the following precedence (command line args > configuration options > default value) - deletes config.rs and the former implementation that loads configuration options into a Simulation- Config struct

- The latter allows dynamic, programmatic updates to the log level at runtime. - The replace was necessary to capture and log command line arguments and configuration options which may or may not contain specification of the log levels after parsing

- additionally reduces the LOC by implementing the overwrite_field!(...) macro and from_config_field(...) generic function. - makes the requirement of a configuration file optional, defaulting to default Cli values if no config file is provided

carlaKC · 2024-05-06T19:02:26Z

Needs rebase!

carlaKC · 2024-05-15T18:53:23Z

Since this is pretty stale now and needs a rebase anyway, let's split it out into a change for just adding a simple config struct and another for a config file - as originally flagged in early review. Would also be nice to have some of the default mechanics in place that we have for the sim file (automatically detect a default value, for example).

carlaKC · 2024-06-24T12:19:05Z

Closing due to inactivity, please feel free to re-open if you've got capacity to pick this back up with the suggested split.

enigbe requested review from sr-gi, okjodom and carlaKC February 1, 2024 22:16

enigbe self-assigned this Feb 1, 2024

carlaKC reviewed Feb 2, 2024

View reviewed changes

enigbe force-pushed the feat-add-simulation-config branch from 6bf8f83 to 237475f Compare February 9, 2024 02:51

enigbe requested a review from carlaKC February 9, 2024 03:14

carlaKC reviewed Feb 12, 2024

View reviewed changes

enigbe added 6 commits March 11, 2024 12:47

chore: format code to wrap at max width of 120

590d834

refactor: make log interval configurable

2f816b6

enigbe force-pushed the feat-add-simulation-config branch from defc4f5 to 2f816b6 Compare March 11, 2024 12:32

docs: update README with note on config file

5f5e075

enigbe requested a review from carlaKC March 11, 2024 13:42

adamjonas mentioned this pull request Mar 18, 2024

feat: add simulation config adamjonas/sim-ln#1

Open

This was referenced Mar 18, 2024

feat: add simulation config Extheoisah/sim-ln#1

Closed

feat: add simulation config Extheoisah/sim-ln#2

Closed

carlaKC removed request for sr-gi, okjodom and carlaKC May 20, 2024 19:24

carlaKC closed this Jun 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add simulation config #166

feat: add simulation config #166

enigbe commented Feb 1, 2024 •

edited

Loading

carlaKC left a comment

carlaKC Feb 2, 2024

enigbe Feb 9, 2024

carlaKC Feb 12, 2024

okjodom commented Feb 9, 2024

carlaKC commented Feb 12, 2024 •

edited

Loading

carlaKC left a comment

carlaKC Feb 12, 2024

carlaKC Feb 12, 2024

enigbe Mar 11, 2024

carlaKC Feb 12, 2024

carlaKC Feb 12, 2024

enigbe Mar 11, 2024

carlaKC commented May 6, 2024

carlaKC commented May 15, 2024

carlaKC commented Jun 24, 2024

feat: add simulation config #166

feat: add simulation config #166

Conversation

enigbe commented Feb 1, 2024 • edited Loading

What this PR does

Related Issue(s)

Update

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okjodom commented Feb 9, 2024

carlaKC commented Feb 12, 2024 • edited Loading

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented May 6, 2024

carlaKC commented May 15, 2024

carlaKC commented Jun 24, 2024

enigbe commented Feb 1, 2024 •

edited

Loading

carlaKC commented Feb 12, 2024 •

edited

Loading