build: make log level depend on input

[nimrod-starkware]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 52.63158% with 9 lines in your changes missing coverage. Please review.

Project coverage is 71.03%. Comparing base (1d383c5) to head (181d629).
Report is 4 commits behind head on main.

Files	Patch %	Lines
crates/committer_cli/src/parse_input/raw_input.rs	63.63%	4 Missing ⚠️
crates/committer/src/block_committer/input.rs	40.00%	3 Missing ⚠️
crates/committer_cli/src/commands.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #313      +/-   ##
==========================================
+ Coverage   70.59%   71.03%   +0.43%     
==========================================
  Files          38       38              
  Lines        2095     2154      +59     
  Branches     2095     2154      +59     
==========================================
+ Hits         1479     1530      +51     
- Misses        546      558      +12     
+ Partials       70       66       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

Benchmark movements:
full_committer_flow performance regressed!
full_committer_flow time: [28.902 ms 28.977 ms 29.062 ms]
change: [+1.1315% +1.4369% +1.7635%] (p = 0.00 < 0.05)
Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severe

[nimrod-starkware]

python side PR

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

[nimrod-starkware]

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

---

crates/committer/src/block_committer/input.rs line 70 at r1 (raw file):

            40 => LevelFilter::Error,
            _ => panic!("Unexpected log level."),
        };

@dorimedini-starkware, do you know a better way to do that?

Code quote:

    pub fn new(warn_on_trivial_modifications: bool, python_logger_level: usize) -> Self {
        let log_level = match python_logger_level {
            10 => LevelFilter::Debug,
            20 => LevelFilter::Info,
            30 => LevelFilter::Warn,
            40 => LevelFilter::Error,
            _ => panic!("Unexpected log level."),
        };

[dorimedini-starkware]

Reviewed 6 of 7 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nimrod-starkware)

---

crates/committer/src/block_committer/input.rs line 70 at r1 (raw file):

Previously, nimrod-starkware wrote…

@dorimedini-starkware, do you know a better way to do that?

1. what about NOTSET and CRITICAL python levels? if you can set it in python, it should be accepted here.
2. since you use "magic numbers" in the test below, I would even suggest you add a PythonLogLevel enum, and add to/from conversions for usize. You can also add conversion to LevelFilter, to end up with something like this:

pub fn new(warn_on_trivial_modifications: bool, python_logger_level: PythonLogLevel)
    Self {
        warn_on_trivial_modifications,
        log_level: python_logger_level.into(),
    }
}

and to construct a ConfigImpl from python input (for example):

ConfigImpl::new(warn_on_trivial, python_level_usize.into());

and in tests, use the enum explicitly so it's clear to the reader:

config: ConfigImpl::new(true, PythonLogLevel::Debug),

---

crates/committer/src/block_committer/input.rs line 69 at r2 (raw file):

            30 => LevelFilter::Warn,
            40 => LevelFilter::Error,
            _ => panic!("Unexpected log level."),

I wouldn't want to ever panic due to bad logger settings... worst case log a warning/error and set the level to DEBUG

Code quote:

_ => panic!("Unexpected log level."),

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/skeleton_forest_test.rs line 168 at r2 (raw file):

        contracts_trie_root_hash: HashOutput(Felt::from(861_u128 + 248_u128)),
        classes_trie_root_hash: HashOutput(Felt::from(155_u128 + 248_u128)),
        config: ConfigImpl::new(true, 10),

see above

Code quote:

10

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/skeleton_forest_test.rs line 290 at r2 (raw file):

        &input.state_diff.actual_classes_updates(),
        &forest_sorted_indices,
        &ConfigImpl::new(false, 20),

see above

Code quote:

20

[nimrod-starkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/committer/src/block_committer/input.rs line 70 at r1 (raw file):

Previously, dorimedini-starkware wrote…

1. what about NOTSET and CRITICAL python levels? if you can set it in python, it should be accepted here.
2. since you use "magic numbers" in the test below, I would even suggest you add a PythonLogLevel enum, and add to/from conversions for usize. You can also add conversion to LevelFilter, to end up with something like this:

pub fn new(warn_on_trivial_modifications: bool, python_logger_level: PythonLogLevel)
    Self {
        warn_on_trivial_modifications,
        log_level: python_logger_level.into(),
    }
}

and to construct a ConfigImpl from python input (for example):

ConfigImpl::new(warn_on_trivial, python_level_usize.into());

and in tests, use the enum explicitly so it's clear to the reader:

config: ConfigImpl::new(true, PythonLogLevel::Debug),

I changed RawConfigImpl to:

pub(crate) struct RawConfigImpl {
    warn_on_trivial_modifications: bool,
    log_level: PythonLogLevel,
}

where

#[derive(Deserialize_repr, Debug, Serialize)]
#[repr(usize)]
/// Describes a log level https://docs.python.org/3/library/logging.html#logging-levels
pub(crate) enum PythonLogLevel {
    NotSet = 0,
    Debug = 10,
    Info = 20,
    Warning = 30,
    Error = 40,
    Critical = 50,
}

and added impl From<RawConfigImpl> for ConfigImpl

---

crates/committer/src/block_committer/input.rs line 69 at r2 (raw file):

Previously, dorimedini-starkware wrote…

I wouldn't want to ever panic due to bad logger settings... worst case log a warning/error and set the level to DEBUG

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/skeleton_forest_test.rs line 168 at r2 (raw file):

Previously, dorimedini-starkware wrote…

see above

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/skeleton_forest_test.rs line 290 at r2 (raw file):

Previously, dorimedini-starkware wrote…

see above

Done.

[github-actions]

Benchmark movements:
full_committer_flow performance regressed!
full_committer_flow time: [28.916 ms 28.954 ms 28.995 ms]
change: [+1.0659% +1.2631% +1.4673%] (p = 0.00 < 0.05)
Performance has regressed.
Found 8 outliers among 100 measurements (8.00%)
1 (1.00%) low mild
6 (6.00%) high mild
1 (1.00%) high severe

[dorimedini-starkware]

Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware)

---

crates/committer/src/block_committer/input.rs line 66 at r3 (raw file):

        }
    }
}

we do we not like this...? I see no reason to protect these members...
out of scope (requires changing all ::new and all getter method usages) but good to keep in mind,
non-blocking

Suggestion:

#[derive(Debug, Eq, PartialEq)]
pub struct ConfigImpl {
    pub warn_on_trivial_modifications: bool,
    pub log_level: LevelFilter,
}

---

crates/committer_cli/src/parse_input/raw_input.rs line 41 at r3 (raw file):

    Error = 40,
    Critical = 50,
}

what will happen if you send 7 and try to deserialize?
we don't want to panic in this case

Code quote:

#[repr(usize)]
/// Describes a log level https://docs.python.org/3/library/logging.html#logging-levels
pub(crate) enum PythonLogLevel {
    NotSet = 0,
    Debug = 10,
    Info = 20,
    Warning = 30,
    Error = 40,
    Critical = 50,
}

[nimrod-starkware]

Reviewable status: 8 of 10 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/committer_cli/src/parse_input/raw_input.rs line 41 at r3 (raw file):

Previously, dorimedini-starkware wrote…

what will happen if you send 7 and try to deserialize?
we don't want to panic in this case

Now, it will not panic; if an unknown variant is given, it will be set to the Debug level by default.

[nimrod-starkware]

Reviewable status: 8 of 10 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/committer/src/block_committer/input.rs line 66 at r3 (raw file):

Previously, dorimedini-starkware wrote…

we do we not like this...? I see no reason to protect these members...
out of scope (requires changing all ::new and all getter method usages) but good to keep in mind,
non-blocking

The getters are for the Config trait...
But I agree there is no need to keep those fields private..

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.221 ms 34.606 ms 35.069 ms]
change: [+1.9002% +3.2058% +4.5412%] (p = 0.00 < 0.05)
Performance has regressed.
Found 15 outliers among 100 measurements (15.00%)
6 (6.00%) high mild
9 (9.00%) high severe

full_committer_flow performance regressed!
full_committer_flow time: [28.976 ms 29.116 ms 29.367 ms]
change: [+1.1952% +1.7373% +2.5822%] (p = 0.00 < 0.05)
Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
2 (2.00%) high mild
2 (2.00%) high severe

[dorimedini-starkware]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)