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: update temporary speed limit groups import endpoint #9381

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

Sh099078
Copy link
Contributor

@Sh099078 Sh099078 commented Oct 17, 2024

fixes #9399

  • Add an endpoint /infra/{infra_id}/delimited_area that returns the list of track sections inside an area delimited by entry and exit directed locations (i.e. a position on a track and a direction, similar to a light version of a sign with only the relevant positional information).
  • Stop the graph exploration at a specified distance from the entries if an end position is missing. It is considered that and end position is missing if none has been found after exploring the track sections on a specified maximum distance from the entries without having found any exit.
  • Refactorize temporary_speed_limit.rs tests.

@Sh099078 Sh099078 self-assigned this Oct 17, 2024
@github-actions github-actions bot added the area:editoast Work on Editoast Service label Oct 17, 2024
@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2024

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

Codecov Report

Attention: Patch coverage is 88.54806% with 56 lines in your changes missing coverage. Please review.

Project coverage is 38.06%. Comparing base (bae2942) to head (79b6e32).
Report is 210 commits behind head on dev.

Files with missing lines Patch % Lines
editoast/src/views/infra/delimited_area.rs 88.42% 44 Missing ⚠️
editoast/src/views/temporary_speed_limits.rs 92.53% 5 Missing ⚠️
front/src/common/api/generatedEditoastApi.ts 66.66% 3 Missing ⚠️
...nt/src/applications/stdcm/utils/formatStdcmConf.ts 0.00% 2 Missing ⚠️
editoast/src/infra_cache/graph.rs 83.33% 1 Missing ⚠️
front/src/applications/stdcm/hooks/useStdcmEnv.tsx 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    #9381      +/-   ##
============================================
- Coverage     39.62%   38.06%   -1.57%     
============================================
  Files          1300      995     -305     
  Lines         99158    91661    -7497     
  Branches       3282     1174    -2108     
============================================
- Hits          39294    34888    -4406     
+ Misses        57931    56319    -1612     
+ Partials       1933      454    -1479     
Flag Coverage Δ
core ?
editoast 73.26% <89.47%> (-0.05%) ⬇️
front 20.09% <57.14%> (+9.85%) ⬆️
gateway 2.18% <ø> (-0.02%) ⬇️
osrdyne 3.28% <ø> (-0.01%) ⬇️
railjson_generator 87.49% <ø> (ø)
tests 86.74% <ø> (+0.03%) ⬆️

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.


🚨 Try these New Features:

@Sh099078 Sh099078 force-pushed the lf/update-temporary-speed-limits-import branch 12 times, most recently from 0a87049 to 2973f9f Compare October 24, 2024 15:06
@Sh099078 Sh099078 changed the title Update temporary speed limit groups import endpoint editoast: update temporary speed limit groups import endpoint Oct 24, 2024
@Sh099078 Sh099078 force-pushed the lf/update-temporary-speed-limits-import branch 2 times, most recently from d5ea850 to f97338e Compare October 24, 2024 21:27
@Sh099078 Sh099078 marked this pull request as ready for review October 24, 2024 21:45
@Sh099078 Sh099078 requested a review from a team as a code owner October 24, 2024 21:45
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

I've just seen the issue. I don't really like the idea of having several endpoints to create the same object. I'd really prefer this to be done on the import side and not in editoast.
When importing, do you have information on the "line" "track" and "pk" of the start and end of the temporary speed limit zone?

Note

Using the InfraCache is really not ideal for an endpoint. It makes the endpoint unscalable because it's stateful. If you keep the endpoint this way, you'll need to modify the Gateway configuration in prod (to redirect to editoast-statefull).

@Khoyo
Copy link
Contributor

Khoyo commented Oct 25, 2024

I've just seen the issue. I don't really like the idea of having several endpoints to create the same object.

wdym? there is only /temporary_speed_limit_group

I'd really prefer this to be done on the import side and not in editoast. When importing, do you have information on the "line" "track" and "pk" of the start and end of the temporary speed limit zone?

We have signals and their location, so we could get that, yes. The import side does not have the full track graph with the various corrections (eg. we do not run the kd-tree), and can't run them easily since it lacks track geometry.

@flomonster
Copy link
Contributor

I've just seen the issue. I don't really like the idea of having several endpoints to create the same object.

wdym? there is only /temporary_speed_limit_group

I mean that we should keep the CRUD endpoints of an object quite simple and close from it's model. Usualy transformation are done by the user (using utility endpoint or by it's own).
Example: Train schedule / Workschedule / Infra / RS...

Since we don't have LRS in OSRD (for now) it's really difficult to import linear objects such as speed limits (all of them). However, all these objects are converted from a start point to an end point into a list of track ranges in the import script and not by editoast. This PR goes against this paradigm, hence my caution.

I'd really prefer this to be done on the import side and not in editoast. When importing, do you have information on the "line" "track" and "pk" of the start and end of the temporary speed limit zone?

We have signals and their location, so we could get that, yes. The import side does not have the full track graph with the various corrections (eg. we do not run the kd-tree), and can't run them easily since it lacks track geometry.

Maybe editoast can help with this, but with a more generic and reusable endpoint 🤷

@Sh099078 Sh099078 force-pushed the lf/update-temporary-speed-limits-import branch 5 times, most recently from da0cb7a to 2425cab Compare October 29, 2024 10:32
@Sh099078 Sh099078 requested a review from a team October 29, 2024 11:05
@Sh099078
Copy link
Contributor Author

Following discussions with @Khoyo and @flomonster the temporary speed limits group endpoint has been split into two:

  • A temporary speed limit groups creation endpoint (unchanged, accepts an LTV with a list of track sections).
  • A /infra/{infra_id}/delimited_area endpoint that returns the list of track sections inside an area delimited by a list of entries and exits (positions on a track with a direction).

@Sh099078 Sh099078 force-pushed the lf/update-temporary-speed-limits-import branch from e9d2af0 to 19f8aa2 Compare November 7, 2024 14:45
@Sh099078 Sh099078 requested a review from a team as a code owner November 7, 2024 16:55
@Sh099078 Sh099078 force-pushed the lf/update-temporary-speed-limits-import branch 2 times, most recently from 0ca14eb to 3200621 Compare November 8, 2024 09:38
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.

Great PR! Not much to say on the process apart from the points we discussed orally a while ago, but that's for another PR, if any.

I believe a few panic!s should be turned into proper errors, and there is probably a few ways to linearize to code using Option<>'s functional interface and the ? operator.

@Sh099078 Sh099078 force-pushed the lf/update-temporary-speed-limits-import branch 6 times, most recently from c37a70b to 12c1193 Compare November 11, 2024 18:25
Copy link
Contributor

@kmer2016 kmer2016 left a comment

Choose a reason for hiding this comment

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

LGTM for front side :)

@Sh099078 Sh099078 force-pushed the lf/update-temporary-speed-limits-import branch 7 times, most recently from 8d84a1a to def2d71 Compare November 20, 2024 09:13
@Sh099078 Sh099078 requested a review from leovalais November 20, 2024 10:40
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.

LGTM. Sorry about the time I took to review this. I hope that didn't block rest of the implementation fo the feature. The last batch of comments / responses:

@github-actions github-actions bot added the area:front Work on Standard OSRD Interface modules label Nov 25, 2024
@Sh099078 Sh099078 force-pushed the lf/update-temporary-speed-limits-import branch 2 times, most recently from 986b709 to d0c7a95 Compare November 25, 2024 14:55
Split the endpoint in two:
    - One endpoint remains responsible from importing temporary speed
      limits and their list of track ranges.
    - A new infra endpoint `delimited_area` returns the list of track
      ranges inside an area delimited by a list of entries and exits
      (directed locations on the tracks, quite similar to the position
      and direction of signs).

Signed-off-by: Loup Federico <[email protected]>
@Sh099078 Sh099078 force-pushed the lf/update-temporary-speed-limits-import branch from d0c7a95 to 79b6e32 Compare November 25, 2024 14:59
@Sh099078 Sh099078 added this pull request to the merge queue Nov 25, 2024
Merged via the queue into dev with commit 2fe67a4 Nov 25, 2024
27 checks passed
@Sh099078 Sh099078 deleted the lf/update-temporary-speed-limits-import branch November 25, 2024 16:13
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 area:front Work on Standard OSRD Interface modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update editoast LTV endpoint to take signals instead of track sections
6 participants