Allow crate users to define and use custom models #8
Conversation
…essor and Storage
…model) as YAML value
… constructors with mutex
…way and StochasticGate
Codecov Report
@@ Coverage Diff @@
## main #8 +/- ##
==========================================
- Coverage 76.77% 75.40% -1.37%
==========================================
Files 21 23 +2
Lines 2058 2094 +36
==========================================
- Hits 1580 1579 -1
- Misses 478 515 +37
Continue to review full report at Codecov.
|
|
This is awesome. Thanks, @kirushyk. I'll review and test soon. @uint, @smallstepman Any thoughts? |
|
While the whole thing looks clever, I'm a bit wary of the overall strategy. It introduces a bit of complexity and overhead that I'm not sure you need here. This is a bit of an opinion, but I generally agree with the idea of not using trait objects for struct internals - it tends to muddy things up and eventually force people to use downcasting and boilerplate. In a way, some of this feels like a partial reimplementation of typetag - I mean particularly the static that holds some information needed to access all the possible trait implementors, and the idea that (I think?) a user of the library would have to An alternative approach might be to make sure pub struct Simulation<M: AsModel> {
models: Vec<M>,
connectors: Vec<Connector>,
messages: Vec<Message>,
services: Services,
}Then a user of the library, if they really need to both add custom models and use our serialization format (this is probably fairly unlikely anyway, isn't it?), can provide their own enum that encompasses every possible model, implements What do you think of that, @kirushyk? |
|
Ah, another thought: we might want to think about making (de)serialization stuff an opt-in feature so that users of the library can choose not to pull all the |
|
Interesting. Notes from my review and testing:
These are some major benefits in my view. Especially point 1. Between (Case A) a simple implementation and complex usage vs. (Case B) complex implementation and simple usage, I think the Case B is best. Custom models will be a very common use case. Of course, simple implementation and simple usage is the best case scenario 🙂 , but this seems like a challenging problem that we may have to decide where to put the complexity.
The future compatibility with typetag is somewhat valuable, but not a big factor. The crate is conceptually based on the Discrete Event System Specification, where models should be treated as black boxes by the simulator, and there is strict separation between the simulator and the models that it simulates. From the article you linked:
The limited flexibility may enforce the good Discrete Event System Specification pattern of "models are black boxes to the simulator" and strictly using predefined interfaces. I wouldn't say the limited flexibility is a benefit, but the Discrete Event System Specification approach makes this a smaller concern.
I believe this would constrain a single simulation to only have models of one type? Is my understanding correct there? Almost all simulations will have a variety of model types involved, so that would be blocking concern.
I think both serialization (for NPM/JavaScript users) and custom models (for crate/Rust users) are important, but not both together. |
|
@kirushyk A side question. Can we have sim_derive as an optional feature/dependency of the sim crate, and re-export SerializableModel within sim? If so, do you think that is a good change? My hypothesis is that a single import of "sim" (with an optional derive feature) will be a better crate user experience. |
| fn get_type(&self) -> &'static str { | ||
| stringify!(#name) | ||
| } | ||
| fn serialize(&self) -> serde_yaml::Value { |
There was a problem hiding this comment.
The serde_yaml::Value is in a few places in the PR diff. Does this limit serde_json serializations at all?
There was a problem hiding this comment.
I checked and serde_yaml::Value works for both YAML and JSON. But serde_json::Value doesn't work out-of-box.
sim_derive/Cargo.toml
Outdated
|
|
||
| [dependencies] | ||
| syn = "*" | ||
| quote = "*" |
There was a problem hiding this comment.
I haven't seen this dependency version format before. Is this preferred over the usual semantic versioning constraints?
There was a problem hiding this comment.
I specified required versions for sync and quote dependencies.
Yes, agreed. This Model trait and custom models stuff is higher priority than optional serde, but valid point. That's a downside. |
|
@kirushyk use std::f64::INFINITY;
use serde::{Deserialize, Serialize};
use sim::input_modeling::random_variable::ContinuousRandomVariable;
use sim::models::model_trait::{AsModel, SerializableModel};
use sim::models::{Generator, Model, ModelMessage};
use sim::simulator::{Connector, Message, Services, Simulation, WebSimulation};
use sim::utils::error::SimulationError;
use sim_derive::SerializableModel;
use wasm_bindgen_test::*;
wasm_bindgen_test_configure!(run_in_browser);
/// The passive model does nothing
#[derive(Debug, Clone, Serialize, Deserialize, SerializableModel)]
#[serde(rename_all = "camelCase")]
pub struct Passive {
ports_in: PortsIn,
}
#[derive(Debug, Clone, Serialize, Deserialize)]
struct PortsIn {
job: String,
}
impl Passive {
pub fn new(job_port: String) -> Self {
Self {
ports_in: PortsIn { job: job_port },
}
}
}
impl AsModel for Passive {
fn status(&self) -> String {
"Passive".into()
}
fn events_ext(
&mut self,
_incoming_message: ModelMessage,
_services: &mut Services,
) -> Result<Vec<ModelMessage>, SimulationError> {
Ok(Vec::new())
}
fn events_int(
&mut self,
_services: &mut Services,
) -> Result<Vec<ModelMessage>, SimulationError> {
Ok(Vec::new())
}
fn time_advance(&mut self, _time_delta: f64) {
// No future events list to advance
}
fn until_next_event(&self) -> f64 {
// No future events list, as a source of finite until_next_event
// values
INFINITY
}
}
#[test]
fn step_n_with_custom_passive_model() {
let models = [
Model::new(
String::from("generator-01"),
Box::new(Generator::new(
ContinuousRandomVariable::Exp { lambda: 0.5 },
None,
String::from("job"),
false,
false,
)),
),
Model::new(
String::from("passive-01"),
Box::new(Passive::new(String::from("job"))),
),
];
let connectors = [Connector::new(
String::from("connector-01"),
String::from("generator-01"),
String::from("passive-01"),
String::from("job"),
String::from("job"),
)];
let mut simulation = Simulation::post(models.to_vec(), connectors.to_vec());
// 1 initialization event, and 2 events per generation
let messages = simulation.step_n(9).unwrap();
let generations_count = messages.len();
let expected = 4; // 4 interarrivals from 9 steps
assert_eq!(generations_count, expected);
}
#[wasm_bindgen_test]
fn step_n_with_custom_passive_model_wasm() {
let models = r#"
- type: "Generator"
id: "generator-01"
portsIn: {}
portsOut:
job: "job"
messageInterdepartureTime:
exp:
lambda: 0.5
- type: "Passive"
id: "passive-01"
portsIn:
job: "job"
"#;
let connectors = r#"
- id: "connector-01"
sourceID: "generator-01"
targetID: "passive-01"
sourcePort: "job"
targetPort: "job"
"#;
let mut simulation = WebSimulation::post_yaml(&models, &connectors);
// 1 initialization event, and 2 events per generation
let messages: Vec<Message> = serde_json::from_str(&simulation.step_n_json(9)).unwrap();
let generations_count = messages.len();
let expected = 4; // 4 interarrivals from 9 steps
assert_eq!(generations_count, expected);
}Error from |
|
@ndebuhr to your test. |
|
I still need to fix error handling to make your test gracefully quit in case deserialization function is not found in list of known models. |
|
@kirushyk Great, thanks! WASM experimentation and testing are good now. I'll wait a couple of days to see if @uint has any additional thoughts. I think we merge this to have a first working version. Then, iterate as opportunities arise for improvement. I'll also write some custom model tests to enable easier future iteration development. |
Codecov Report
@@ Coverage Diff @@
## main #8 +/- ##
==========================================
- Coverage 76.77% 75.11% -1.66%
==========================================
Files 21 23 +2
Lines 2058 2098 +40
==========================================
- Hits 1580 1576 -4
- Misses 478 522 +44
Continue to review full report at Codecov.
|
|
@ndebuhr If you give me a day, I could try to produce an alternative PR, just to have a direct comparison. I really feel there is a better approach there. |
|
@uint Sounds great. The more options we all have to look at together, the better. |
|
I made small fix to handle the case when the model requested wasn't previously registered. |
enum_dispatchapproach is replaced by storinginnerconcrete model inBox<dyn AsModel>.from_valueconstructors. Crate users can register their own custom models.sim_derivecrate added to provide macros for filling template methods.