-
Notifications
You must be signed in to change notification settings - Fork 2
Refer to "dangerous" rather than "broken" options #1224
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,8 +1,8 @@ | ||||||||||||||
| //! Read and validate model parameters from `model.toml`. | ||||||||||||||
| //! | ||||||||||||||
| //! This module defines the `ModelParameters` struct and helpers for loading and | ||||||||||||||
| //! validating the `model.toml` configuration used by the model. Validation | ||||||||||||||
| //! functions ensure sensible numeric ranges and invariants for runtime use. | ||||||||||||||
| //! This module defines the `ModelParameters` struct and helpers for loading and validating the | ||||||||||||||
| //! `model.toml` configuration used by the model. Validation functions ensure sensible numeric | ||||||||||||||
| //! ranges and invariants for runtime use. | ||||||||||||||
| use crate::asset::check_capacity_valid_for_asset; | ||||||||||||||
| use crate::input::{ | ||||||||||||||
| deserialise_proportion_nonzero, input_err_msg, is_sorted_and_unique, read_toml, | ||||||||||||||
|
|
@@ -16,42 +16,42 @@ use std::sync::OnceLock; | |||||||||||||
|
|
||||||||||||||
| const MODEL_PARAMETERS_FILE_NAME: &str = "model.toml"; | ||||||||||||||
|
|
||||||||||||||
| /// The key in `model.toml` which enables known-broken model options. | ||||||||||||||
| /// The key in `model.toml` which enables potentially dangerous model options. | ||||||||||||||
| /// | ||||||||||||||
| /// If this option is present and true, the model will permit certain | ||||||||||||||
| /// experimental or unsafe behaviours that are normally disallowed. | ||||||||||||||
| pub const ALLOW_BROKEN_OPTION_NAME: &str = "please_give_me_broken_results"; | ||||||||||||||
| /// If this option is present and true, the model will permit certain experimental or unsafe | ||||||||||||||
| /// behaviours that are normally disallowed. | ||||||||||||||
| pub const ALLOW_DANGEROUS_OPTION_NAME: &str = "please_give_me_broken_results"; | ||||||||||||||
|
|
||||||||||||||
| /// Global flag indicating whether broken model options have been enabled. | ||||||||||||||
| /// Global flag indicating whether potentially dangerous model options have been enabled. | ||||||||||||||
| /// | ||||||||||||||
| /// This is stored in a `OnceLock` and must be set exactly once during | ||||||||||||||
| /// startup (see `set_broken_model_options_flag`). | ||||||||||||||
| static BROKEN_OPTIONS_ALLOWED: OnceLock<bool> = OnceLock::new(); | ||||||||||||||
| /// This is stored in a `OnceLock` and must be set exactly once during startup (see | ||||||||||||||
| /// [`set_dangerous_model_options_flag`]). | ||||||||||||||
| static DANGEROUS_OPTIONS_ENABLED: OnceLock<bool> = OnceLock::new(); | ||||||||||||||
|
|
||||||||||||||
| /// Return whether broken model options were enabled by the loaded config. | ||||||||||||||
| /// Whether potentially dangerous model options were enabled by the loaded config. | ||||||||||||||
| /// | ||||||||||||||
| /// # Panics | ||||||||||||||
| /// | ||||||||||||||
| /// Panics if the global flag has not been set yet (the flag should be set by | ||||||||||||||
| /// `ModelParameters::from_path` during program initialization). | ||||||||||||||
| pub fn broken_model_options_allowed() -> bool { | ||||||||||||||
| *BROKEN_OPTIONS_ALLOWED | ||||||||||||||
| /// [`ModelParameters::from_path`] during program initialisation). | ||||||||||||||
| pub fn dangerous_model_options_enabled() -> bool { | ||||||||||||||
| *DANGEROUS_OPTIONS_ENABLED | ||||||||||||||
| .get() | ||||||||||||||
| .expect("Broken options flag not set") | ||||||||||||||
| .expect("Dangerous options flag not set") | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /// Set the global flag indicating whether broken model options are allowed. | ||||||||||||||
| /// Set the global flag indicating whether potentially dangerous model options are enabled. | ||||||||||||||
| /// | ||||||||||||||
| /// Can only be called once; subsequent calls will panic (except in tests, where it can be called | ||||||||||||||
| /// multiple times so long as the value is the same). | ||||||||||||||
| fn set_broken_model_options_flag(allowed: bool) { | ||||||||||||||
| let result = BROKEN_OPTIONS_ALLOWED.set(allowed); | ||||||||||||||
| fn set_dangerous_model_options_flag(enabled: bool) { | ||||||||||||||
| let result = DANGEROUS_OPTIONS_ENABLED.set(enabled); | ||||||||||||||
| if result.is_err() { | ||||||||||||||
| if cfg!(test) { | ||||||||||||||
| // Sanity check | ||||||||||||||
| assert_eq!(allowed, broken_model_options_allowed()); | ||||||||||||||
| assert_eq!(enabled, dangerous_model_options_enabled()); | ||||||||||||||
| } else { | ||||||||||||||
| panic!("Attempted to set BROKEN_OPTIONS_ALLOWED twice"); | ||||||||||||||
| panic!("Attempted to set DANGEROUS_OPTIONS_ENABLED twice"); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
@@ -89,9 +89,9 @@ define_param_default!(default_mothball_years, u32, 0); | |||||||||||||
| pub struct ModelParameters { | ||||||||||||||
| /// Milestone years | ||||||||||||||
| pub milestone_years: Vec<u32>, | ||||||||||||||
| /// Allow known-broken options to be enabled. | ||||||||||||||
| /// Allow potentially dangerous options to be enabled. | ||||||||||||||
| #[serde(default, rename = "please_give_me_broken_results")] // Can't use constant here :-( | ||||||||||||||
| pub allow_broken_options: bool, | ||||||||||||||
| pub allow_dangerous_options: bool, | ||||||||||||||
alexdewar marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
| /// The (small) value of capacity given to candidate assets. | ||||||||||||||
| /// | ||||||||||||||
| /// Don't change unless you know what you're doing. | ||||||||||||||
|
|
@@ -169,7 +169,7 @@ fn check_price_tolerance(value: Dimensionless) -> Result<()> { | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| fn check_remaining_demand_absolute_tolerance( | ||||||||||||||
| allow_broken_options: bool, | ||||||||||||||
| dangerous_options_enabled: bool, | ||||||||||||||
| value: Flow, | ||||||||||||||
| ) -> Result<()> { | ||||||||||||||
| ensure!( | ||||||||||||||
|
|
@@ -178,12 +178,12 @@ fn check_remaining_demand_absolute_tolerance( | |||||||||||||
| ); | ||||||||||||||
|
|
||||||||||||||
| let default_value = default_remaining_demand_absolute_tolerance(); | ||||||||||||||
| if !allow_broken_options { | ||||||||||||||
| if !dangerous_options_enabled { | ||||||||||||||
| ensure!( | ||||||||||||||
| value == default_value, | ||||||||||||||
| "Setting a remaining_demand_absolute_tolerance different from the default value of {:e} \ | ||||||||||||||
| is potentially dangerous, set please_give_me_broken_results to true \ | ||||||||||||||
| if you want to allow this.", | ||||||||||||||
| "Setting a remaining_demand_absolute_tolerance different from the default value of \ | ||||||||||||||
| {:e} is potentially dangerous, set {ALLOW_DANGEROUS_OPTION_NAME} to true if you want \ | ||||||||||||||
| to allow this.", | ||||||||||||||
|
Comment on lines
+184
to
+186
|
||||||||||||||
| "Setting a remaining_demand_absolute_tolerance different from the default value of \ | |
| {:e} is potentially dangerous, set {ALLOW_DANGEROUS_OPTION_NAME} to true if you want \ | |
| to allow this.", | |
| "Setting a remaining_demand_absolute_tolerance different from the default value of " | |
| "{:e} is potentially dangerous, set {ALLOW_DANGEROUS_OPTION_NAME} to true if you want " | |
| "to allow this.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
\-continued multi-line error string includes indentation before “To run anyway…”, which will become leading whitespace in the emitted error message. Consider removing the indentation after the continuation (or otherwise building the string) so the user-facing message is formatted cleanly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not actually true; I just checked.
It's odd that Copilot trips over on some quite normal Rust constructs.