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.

Sign up for GitHub

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

Jump to bottom

core: directly load infrastructure from RailJSON #7248

Merged
merged 4 commits into from
Apr 19, 2024
Merged

core: directly load infrastructure from RailJSON #7248

merged 4 commits into from
Apr 19, 2024

Conversation

multun
Copy link
Contributor

@multun multun commented Apr 17, 2024 •
edited by bougue-pe
Loading

Fix part of #7056

First commit is a rebased squash of all PRs previously targeting core/infra_load_refactor branch.

(some tests are still using legacy builder)

@multun multun force-pushed the core/infra_load_refactor branch from f23d594 to c1da19e Compare April 18, 2024 09:28
@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (dev@b3228a3). Click here to learn what that means.
Report is 2 commits behind head on dev.

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

Additional details and impacted files 
@@          Coverage Diff           @@
##             dev    #7248   +/-   ##
======================================
  Coverage       ?   10.91%           
======================================
  Files          ?      601           
  Lines          ?   100477           
  Branches       ?      930           
======================================
  Hits           ?    10965           
  Misses         ?    88587           
  Partials       ?      925
Flag Coverage Δ
front 9.14% <ø> (?)
gateway 2.43% <ø> (?)
railjson_generator 87.49% <ø> (?)
tests 83.96% <ø> (?)

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.

@multun multun force-pushed the core/infra_load_refactor branch 4 times, most recently from 200c6c0 to 4cd84ce Compare April 18, 2024 13:31
@multun multun marked this pull request as ready for review April 18, 2024 13:59
@multun multun requested review from a team as code owners April 18, 2024 13:59
@multun multun requested a review from eckter April 18, 2024 13:59
@multun multun force-pushed the core/infra_load_refactor branch 2 times, most recently from 7e656a3 to a0a4634 Compare April 18, 2024 14:04
Khoyo
Khoyo approved these changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

@Khoyo Khoyo left a comment

Choose a reason for hiding this comment

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

Overall LGTM.

Most of my comments are kinda nitpicky, I'll leave to @multun judgement whether to address them now or later.

The process is globally much more readable than the legacy import, but it still feels kinda hard to follow the big picture. I wonder if some restructuring - or more documentation could help.

It's also unclear why some stuff is done in the infra implementation, in the builder and in the parse function - I'm guessing this is still kinda WIP :)

:shipit:

core/src/main/kotlin/fr/sncf/osrd/sim_infra_adapter/RawInfraRJSParser.kt Outdated Show resolved Hide resolved
core/src/main/kotlin/fr/sncf/osrd/sim_infra_adapter/RawInfraRJSParser.kt Outdated Show resolved Hide resolved
core/src/main/kotlin/fr/sncf/osrd/sim_infra_adapter/RawInfraRJSParser.kt Outdated Show resolved Hide resolved
core/src/main/kotlin/fr/sncf/osrd/sim_infra_adapter/RawInfraRJSParser.kt Outdated Show resolved Hide resolved
core/src/main/kotlin/fr/sncf/osrd/sim_infra_adapter/RawInfraRJSParser.kt Outdated Show resolved Hide resolved
core/src/main/kotlin/fr/sncf/osrd/sim_infra_adapter/RawInfraRJSParser.kt Outdated Show resolved Hide resolved
core/src/main/kotlin/fr/sncf/osrd/sim_infra_adapter/RawInfraRJSParser.kt Outdated Show resolved Hide resolved
...srd-sim-infra/src/main/kotlin/fr/sncf/osrd/sim_infra/impl/new_impl/RawInfraFromRjsBuilder.kt Outdated Show resolved Hide resolved
@multun multun force-pushed the core/infra_load_refactor branch 2 times, most recently from 5fbfaef to 10745a7 Compare April 18, 2024 23:57
@multun multun requested a review from Khoyo April 19, 2024 00:01
@multun multun force-pushed the core/infra_load_refactor branch 2 times, most recently from d239a6b to bce3faa Compare April 19, 2024 01:13
@multun multun force-pushed the core/infra_load_refactor branch from bce3faa to 89c91b6 Compare April 19, 2024 09:12
@multun multun force-pushed the core/infra_load_refactor branch from 89c91b6 to 5b5d931 Compare April 19, 2024 09:15
@multun multun force-pushed the core/infra_load_refactor branch from 5b5d931 to ebbeb01 Compare April 19, 2024 11:53
Erashin
Erashin approved these changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@Erashin Erashin left a comment

Choose a reason for hiding this comment

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

I globally agree with @younesschrifi. This is a very good step (and an essential one, goodbye java infra) towards the right direction. Good job :)

...srd-sim-infra/src/main/kotlin/fr/sncf/osrd/sim_infra/impl/new_impl/RawInfraFromRjsBuilder.kt Outdated Show resolved Hide resolved
@multun multun force-pushed the core/infra_load_refactor branch from ebbeb01 to f2d4583 Compare April 19, 2024 13:30
bougue-pe and others added 2 commits April 19, 2024 15:34
@bougue-pe @multun
core: directly import infrastructure from RailJSON
ffafe01 
- removes SimInfraAdapter, which was a leftover of the development of the adapter.
  the old adapter now directly returns a RawInfra
- add a new builder and RawInfra implementation

Co-Authored-By: Victor Collod <victor.collod@epita.fr>
@multun @bougue-pe
core: remove the old infrastructure API
3aeba40 
This change also removes:
- the old adapter
- the tooling to compare instructures

Co-Authored-By: Pierre-Etienne Bougué <bougue.pe@proton.me>
@multun multun force-pushed the core/infra_load_refactor branch from f2d4583 to 8873c87 Compare April 19, 2024 13:36
@multun multun removed the request for review from eckter April 19, 2024 13:39
eckter
eckter approved these changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@eckter eckter left a comment

Choose a reason for hiding this comment

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

LGTM, I skimmed through the first commit but I did review large parts of it

I tested on STDCM-related stuff, there's a new bug but it's not related to this specific PR (something with conditional speed limits)

I had some weird stuff with gradle / intellij at first when moving to this branch, something about broken cache files. But it fixed itself before I could figure out what was going on.

clarani
clarani approved these changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

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

LGTM, tested ✅

@multun multun added this pull request to the merge queue Apr 19, 2024
Merged via the queue into dev with commit 00215fe Apr 19, 2024
17 checks passed
@multun multun deleted the core/infra_load_refactor branch April 19, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bougue-pe bougue-pe bougue-pe left review comments

@Erashin Erashin Erashin approved these changes

@clarani clarani clarani approved these changes

@leovalais leovalais leovalais approved these changes

@eckter eckter eckter approved these changes

@Khoyo Khoyo Khoyo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@multun @codecov-commenter @Khoyo @eckter @leovalais @clarani @Erashin @bougue-pe