editoast: use uom to type units

[Tristramg]

Closes #9431

This pull request isn’t ready, and was made to be a base for discussions. There are many values that still need to be typed

The models are hard to convert (I’m struggling with derive(Model). But maybe it’s the right level of separation? In the database they are stored as floats, so maybe building the unit values in impl From<RollingStockModel> for LightRollingStock is actually the right place?

We get very long lines, such as Acceleration::new::<meter_per_second_squared>(startup_acceleration). But I think it’s a good thing to have very explicit units.

In general I like that the units are now very explicit, and we no longer need comments to indicate what to do, and we can even remove them completly.

What do you think in general?

[leovalais]

Looks great! 😃

We get very long lines, such as Acceleration::new::<meter_per_second_squared>(startup_acceleration). But I think it’s a good thing to have very explicit units.

I strongly agree. This makes reading the code much easier.

The models are hard to convert (I’m struggling with derive(Model). But maybe it’s the right level of separation? In the database they are stored as floats, so maybe building the unit values in impl From for LightRollingStock is actually the right place?

I'd rather have the uom quantities directly in the model. I believe you can use #[model(remote = "f64")] above the fields to instruct the macro to generate a few .into()s at the right places. This won't work if uom::Quantity doesn't implement From<f64> or if non-base units are stored. In which case we either need an adapter, or a new field transformation in Model like #[model(remote = f64, unit = millimeter)]. Wdyt about that? I can help with the derive macro, it's fairly easy to add such transformations.

---

I hope we can have this working as its makes everything so much clearer!

(Also we're in editoast around here, not in core, cf. PR title 😛)

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 95.87302% with 13 lines in your changes missing coverage. Please review.

Project coverage is 79.94%. Comparing base (6a105bf) to head (89a22df).

Files with missing lines	Patch %	Lines
editoast/src/views/timetable/stdcm/request.rs	50.00%	6 Missing ⚠️
editoast/editoast_derive/src/annotate_units.rs	93.02%	3 Missing ⚠️
editoast/editoast_derive/src/model/parsing.rs	72.72%	3 Missing ⚠️
editoast/editoast_derive/src/model/args.rs	0.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff            @@
##              dev    #9928    +/-   ##
========================================
  Coverage   79.93%   79.94%            
========================================
  Files        1057     1061     +4     
  Lines      106302   106472   +170     
  Branches      724      724            
========================================
+ Hits        84977    85116   +139     
- Misses      21283    21314    +31     
  Partials       42       42            

Flag	Coverage Δ
editoast	73.96% <95.87%> (+0.04%)	⬆️
front	89.19% <ø> (ø)
gateway	2.18% <ø> (ø)
osrdyne	3.28% <ø> (ø)
railjson_generator	87.50% <ø> (ø)
tests	87.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[Tristramg]

@leovalais thank you for the feedback. I tried to fiddle with macro by myself, but I have little experience here. See the commit 590a971 for the change in the Model macro and an example on one field

If it’s ok for you, I’ll go brrr on all the fields of rolling stock.

I’m a bit worried about the interface with the http part: isn’t there a risk that serde will use the wrong unit?

[woshilapin]

OK, in general, very interesting to discover how the integration of uom takes shape in a real project. After reading the entire PR, I now understand why fields are defined as Length and not Length<meter>: it doesn't really matter how it is stored. What is important is how you put it inside (and the new function needs the unit) and how you get it out (and the get function & friends need units).

That said, even if I now understand the technical reason, which seems to be a great design of the API, I'm still wondering how it will work in practice. As a developer, I'll probably still ask "what does editoast takes as inputs units?" then take a look at the endpoint, then the structs and look inside the struct, failing to find the information.

Introduction of uom seems to change the approach:

• before, we consider that a length is always in meter through the entire system (but not typed, hence we had bugs, but that's not the point here)
• with uom, we don't care how a length is stored, we care how it's read, so we might take millimeter on an endpoint, meter on another endpoint. Sure, in the end, everything is typed inside editoast, but not sure it's going to help our API consumer to get the correct information inside the system.

Not sure what to make of that information. Happy to discuss at some point.

[Tristramg]

https://github.com/OpenRailAssociation/osrd/actions/runs/12177764809/job/33967384585
Those checks on openapi will be helpful to me sure we didn’t miss anything in input/output (and check the documentation). Nice work!

[leovalais]

The macro changes look fine to me 😄

I have not tested, but I'm pretty sure that the serialization to core and the deserialization from frontend requests will break. uom only uses base units in these cases, and that's not configurable on their end. We'll likely need adapters using serde(remote. These could be placed into editoast_common, wdyt? @Tristramg @woshilapin

#[derive(Debug, Deserialize, Serialize)]
#[serde(remote = "uom::si::f64::Length")]
struct Millimeters(#[serde(getter = "uom::si::f64::Length::get::<millimeter>")] f64);

impl From<Millimeters> for uom::si::f64::Length {
    fn from(mm: Millimeters) -> Self {
        uom::si::f64::Length::new::<millimeter>(mm.0)
    }
}

#[derive(Debug, Deserialize, Serialize)]
pub struct UndirectedTrackRange {
    pub track_section: String,
    #[serde(with = "Millimeters")]
    pub begin: uom::si::f64::Length,
    #[serde(with = "Millimeters")]
    pub end: uom::si::f64::Length,
}

I guess this also answers @woshilapin question about where to get the unit information from editoast API.

[Tristramg]

Thank you for the feedback.

Here are the main updates, the big stuff being in 0b1c9fc

I wanted to make a macro to generate all the structures, and it interfered with serde(remote) and serde(getter). I’m not sure what was happing. So I used a mod to be able to deserialize our structures.

I also added converters in the mod to simplify a bit the usage in the code.

I’m not so happy with the way to handle Option, but it’s probably ok in the end.

[leovalais]

Too bad serde remote doesn't work, but the macro_rules is not that complicated and it's straightforward to support new units.

As we discussed, we need to document the units in the OpenAPI. It'd be great to have some mechanism to avoid repeating the unit in a doc comment, and instruct utoipa to use a float when necessary. But it may require a fair amount of work to do that using metaprog. It's probably fine to repeat the unit in a comment, we'll just have to be extra careful when reviewing that the documented unit matches the one used in serde(with =.

[Tristramg]

🥳 🎉 d5e9940 🎈

[leovalais]

So nice! I didn't know we could do that using proc-macros! (I guess Rust macro system isn't that bad after all... 😁) Thanks for keeping digging this, it's much better than manual comments!

Maybe a comment would be useful in units.rs above the define_units!(...) indicating that the new unit should also be added to the proc-macro hash map, and vice versa.

Or to avoid having units defined in two place, maybe define_unit! could define a doc! macro inside the module that expands to the doc literal, and call that macro in #[doc = #unit::doc!()]. Not too sure about this one, but it would make adding units easier. Wdyt?

[Tristramg]

I was thinking calling from editost_derive the properties from editoast_common, but wouldn’t that defeat the separation of crates?

We might, maybe, also pull the information from the uom crate, but I got stock when trying to transform the string into the path

[leovalais]

I'm not suggesting to "use" definitions from common in derive, just rather to expect in the macro expansion some definitions to be "there". What I'm suggesting would be something in the lines of:

// in common

define_unit!(Length, meter, "Length in m");
// expanding to
mod meter {
	pub fn serialize() { todo!() }
	// ...
	macro_rules! doc { () => { "Length in m" } }
	pub use doc!;
}

// and then, in derive
for
	for
		...
			f.attrs.push(quote! { #[doc = #unit::doc!()] });

I'm not sure what you mean by "also pull the information from the uom crate", does the crate already provide the units abbreviations?

[Tristramg]

A new iteration, from my point of view, this is mergeable (I just need to squash commits, I tried to keep separated the specific changes.

In particular: in common 97fb3f0 it adds a hashing function and the type that is used to store the data. The goal is to maybe use a rational value on the long term? (plus it helps for some troubles I had with Options)

In derive, we now pass the unit module defined in derive instead of the unit from uom. This allows to use the ReprTypedefined in common making thing SOOOO much easier for Option c95441a

What do you think? Do you think other things could be improved or are we good to go?

[leovalais]

A few comments about details. I tried to make sure the units were consistent with the old definitions, but we'll have to test the application thoroughly to ensure no regression occurs... 😓

Thanks for setting all this up!

[leovalais]

LGTM, thank you!

I'll fully test the application tomorrow to try to spot regressions. But I don't think I'm the most qualified to spot them efficiently. Maybe requesting a review from @OpenRailAssociation/osrd-physics and @OpenRailAssociation/osrd-core would be a good idea?