Skip to content
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

editoast: extract osm_to_railjson in separate binary #10215

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

Tristramg
Copy link
Contributor

This is more to open a discussion:

As compile time are unbearable, we could split the editoast binary into smaller ones. As we already started separating crates, this could be quite simple.

In this pull request, osm-to-railjson in separated.

It now must be called as cargo run -p osm_to_railjson

The improvement are not spectacular:

Now:
- 541 units
- total build time: 175.5s
- editoast bin: 109s
- minor change build time: 25s
- binary size (debug): 312Mb

Before:
- 561 units
- total build time: 182.4s
- editoast bin: 109s
- minor change build time: 26secs
- binary size (debug): 314Mb

I’m not sure if it would make sens to extract the other commands that are more close to the editoast core?

@github-actions github-actions bot added the area:editoast Work on Editoast Service label Jan 1, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 1, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 81.58%. Comparing base (00df2a1) to head (b72c427).
Report is 6 commits behind head on dev.

Files with missing lines Patch % Lines
editoast/osm_to_railjson/src/main.rs 0.00% 7 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #10215      +/-   ##
==========================================
- Coverage   81.67%   81.58%   -0.10%     
==========================================
  Files        1071     1041      -30     
  Lines      106118   104586    -1532     
  Branches      727      727              
==========================================
- Hits        86673    85322    -1351     
+ Misses      19406    19225     -181     
  Partials       39       39              
Flag Coverage Δ
editoast 73.72% <0.00%> (-0.04%) ⬇️
front 89.36% <ø> (ø)
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator ?
tests 87.05% <ø> (ø)

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 Tristramg self-assigned this Jan 3, 2025
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need an exhaustive search for osm_to_railjson in our repos to see if it's used somewhere (documentation, CI scripts, deployment, charts, etc.). But in general, I'd say it'd be nice that the binaries are separated, it might indeed help a bit the compilation times (especially not wasting time compiling dependencies that are not used.

@Tristramg Tristramg force-pushed the separate_osm_to_railjson_bin branch 2 times, most recently from 0b3c837 to 9bf92ed Compare January 16, 2025 17:02
@Tristramg Tristramg marked this pull request as ready for review January 16, 2025 17:02
@Tristramg Tristramg requested a review from a team as a code owner January 16, 2025 17:02
@Castavo
Copy link
Contributor

Castavo commented Jan 17, 2025

Isn't splitting the binary for osm-to-railjson especially beneficial to osm-to-railjson ? This binary alone must be much faster to compile now, isn't it ?

@Tristramg Tristramg force-pushed the separate_osm_to_railjson_bin branch from 9bf92ed to b72c427 Compare January 17, 2025 10:15
@Tristramg
Copy link
Contributor Author

Il did a grep on my sncf folder with different projects for osm-to-railjson and osm_to_railjson and didn’t found anything. Maybe it’s used somewhere else I’m not aware of? I don’t think as it’s quite a nice use

@Castavo indeed :) but the much more people working on the general editoast than osm-to-railjson that hasn’t changed in over a year

Copy link
Contributor

@leovalais leovalais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Tristramg Tristramg added this pull request to the merge queue Jan 17, 2025
Merged via the queue into dev with commit 0c08ccf Jan 17, 2025
27 checks passed
@Tristramg Tristramg deleted the separate_osm_to_railjson_bin branch January 17, 2025 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:editoast Work on Editoast Service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants