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: return conflicts list when STDCM request fails #9327

Merged

Conversation

hamz2a
Copy link
Contributor

@hamz2a hamz2a commented Oct 14, 2024

closes #8639

@hamz2a hamz2a requested a review from a team as a code owner October 14, 2024 08:17
@hamz2a hamz2a marked this pull request as draft October 14, 2024 08:17
@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2024

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

Codecov Report

Attention: Patch coverage is 87.27915% with 36 lines in your changes missing coverage. Please review.

Project coverage is 39.52%. Comparing base (5977531) to head (a19b328).
Report is 8 commits behind head on dev.

Files with missing lines Patch % Lines
editoast/src/views/timetable/stdcm.rs 86.95% 36 Missing ⚠️

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

Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #9327      +/-   ##
============================================
+ Coverage     39.07%   39.52%   +0.45%     
  Complexity     2270     2270              
============================================
  Files          1308     1308              
  Lines         99317    99588     +271     
  Branches       3316     3316              
============================================
+ Hits          38805    39363     +558     
+ Misses        58547    58260     -287     
  Partials       1965     1965              
Flag Coverage Δ
core 74.99% <ø> (ø)
editoast 73.24% <87.23%> (+1.32%) ⬆️
front 10.35% <ø> (+0.03%) ⬆️
gateway 2.50% <ø> (ø)
osrdyne 3.29% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 86.71% <100.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.

@hamz2a hamz2a force-pushed the hai/editoast-explanation-of-why-a-simulation-is-not-feasible branch from bb12f6c to eca6a69 Compare October 15, 2024 13:19
@github-actions github-actions bot added the area:editoast Work on Editoast Service label Oct 15, 2024
@hamz2a hamz2a marked this pull request as ready for review October 15, 2024 13:22
@hamz2a hamz2a force-pushed the hai/editoast-explanation-of-why-a-simulation-is-not-feasible branch 2 times, most recently from 6933fe5 to b62d9b5 Compare October 16, 2024 09:46
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.

Again some more debuggability information send back to the front, I love it 😍

In this PR, there is some renaming interlaced with changes of functionality. It would have been nice to first do the renaming in an independent commit to ease the review of the important part (note that the renaming is nice, just not a change that need attentive reviewing).

editoast/src/views/timetable/stdcm.rs Outdated Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Outdated Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Outdated Show resolved Hide resolved
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 for this PR! Left a few comments.

On a more general note, the /stdcm endpoint is starting to do a whole lot of things and this process isn't documented anywhere. The endpoint documentation is just

/// Compute a STDCM and return the simulation result

which isn't exact anymore following your changes.

I'd suggest:

  • documenting the whole process in the handler's documentation with the "happy path" and what we're doing if the simulation fails, making sure to separate both
  • continue the series of scoping comments // n. Nth step in your additions, as it's really helpful to figure out where we're at in the global process
  • split the individual steps into documented functions (you don't have to do that for existing steps, but it would be a nice thing to do for your changes so that we can start enforcing a better modularity from now on)

Wdyt?

editoast/src/views/timetable/stdcm.rs Outdated Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Outdated Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Outdated Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Outdated Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Outdated Show resolved Hide resolved
@hamz2a hamz2a force-pushed the hai/editoast-explanation-of-why-a-simulation-is-not-feasible branch 2 times, most recently from 13be772 to 97bb368 Compare October 17, 2024 13:24
@hamz2a hamz2a force-pushed the hai/editoast-explanation-of-why-a-simulation-is-not-feasible branch 2 times, most recently from 0339850 to 6dfbf6d Compare October 17, 2024 13:49
@hamz2a hamz2a requested a review from a team as a code owner October 17, 2024 13:49
@hamz2a hamz2a force-pushed the hai/editoast-explanation-of-why-a-simulation-is-not-feasible branch from 6dfbf6d to dbe3e20 Compare October 17, 2024 13:51
@hamz2a hamz2a force-pushed the hai/editoast-explanation-of-why-a-simulation-is-not-feasible branch 3 times, most recently from b97ff1d to b780426 Compare October 17, 2024 14:12
tests/tests/test_stdcm.py Outdated Show resolved Hide resolved
@hamz2a hamz2a force-pushed the hai/editoast-explanation-of-why-a-simulation-is-not-feasible branch from b780426 to 70e91cc Compare October 17, 2024 14:41
@hamz2a hamz2a force-pushed the hai/editoast-explanation-of-why-a-simulation-is-not-feasible branch from 70e91cc to 569d51c Compare October 18, 2024 08:22
@hamz2a hamz2a requested a review from eckter October 18, 2024 08:22
Copy link
Contributor

@shenriotpro shenriotpro 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 tests/

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.

Sorry for this second pass of review, but I feel we lack some improvements of code architecture. Let's see together if we can make the code more simple and maintainable.

editoast/src/views/timetable/stdcm.rs Outdated Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Outdated Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Show resolved Hide resolved
editoast/src/views/timetable/stdcm.rs Show resolved Hide resolved
@hamz2a hamz2a force-pushed the hai/editoast-explanation-of-why-a-simulation-is-not-feasible branch 2 times, most recently from 3012e7b to 4ecb4de Compare October 23, 2024 14: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, the remaining comments will be addressed in a separate PR with a refactoring of the endpoint and its utilities.

@hamz2a hamz2a requested review from leovalais and removed request for flomonster October 24, 2024 07:59
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.

Let's merge this one and address all the unaddressed comments in the follow-up PR.

@hamz2a hamz2a added this pull request to the merge queue Oct 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 24, 2024
@hamz2a hamz2a added this pull request to the merge queue Oct 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 24, 2024
Signed-off-by: hamz2a <atrari.hamza@gmail.com>
@hamz2a hamz2a force-pushed the hai/editoast-explanation-of-why-a-simulation-is-not-feasible branch from 4ecb4de to a19b328 Compare October 24, 2024 10:12
@hamz2a hamz2a enabled auto-merge October 24, 2024 10:15
@hamz2a hamz2a added this pull request to the merge queue Oct 24, 2024
Merged via the queue into dev with commit fdf4f01 Oct 24, 2024
24 checks passed
@hamz2a hamz2a deleted the hai/editoast-explanation-of-why-a-simulation-is-not-feasible branch October 24, 2024 10:47
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.

Editoast: explanation of why the simulation is not feasible
7 participants