Make sure that scheduler send all events when jobs are finished

equinor / ert

ERT - Ensemble based Reservoir Tool - is designed for running ensembles of dynamical models such as reservoir models, in order to do sensitivity analysis and data assimilation. ERT supports data assimilation using the Ensemble Smoother (ES), Ensemble Smoother with Multiple Data Assimilation (ES-MDA) and Iterative Ensemble Smoother (IES).

https://ert.readthedocs.io/en/latest/

GNU General Public License v3.0

101 stars 104 forks source link

Make sure that scheduler send all events when jobs are finished #8109

Closed xjules closed 1 month ago

xjules commented 3 months ago

Issue Improves symptoms of https://github.com/equinor/ert/issues/8105

Approach We include scheduler._events.join() and SENTINEL object to make sure that the events left in the qeueu will be proccessed once the jobs are done. To makes sure we won't wait for the queue and SENTINEL indefinitely this introduces self._queue_timeout attribute.

[x] PR title captures the intent of the changes, and is fitting for release notes.
[x] Added appropriate release note label
[x] Commit history is consistent and clean, in line with the contribution guidelines.
[ ] Make sure tests pass locally (after every commit!)

When applicable

[ ] When there are user facing changes: Updated documentation
[ ] New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
[ ] Large PR: Prepare changes in small commits for more convenient review
[ ] Bug fix: Add regression test for the bug
[ ] Bug fix: Create Backport PR to latest release

codecov-commenter commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.88%. Comparing base (abb9957) to head (b7044ad).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #8109 +/- ## ========================================== - Coverage 86.88% 86.88% -0.01% ========================================== Files 379 379 Lines 23972 23976 +4 Branches 634 632 -2 ========================================== + Hits 20828 20831 +3 + Misses 3070 3067 -3 - Partials 74 78 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

berland commented 2 months ago

Squash the commits? The explanation under "Approach" should also be in the git commit message.

berland commented 2 months ago

nitpick: First line in git commit message should be less than 50 chars, and the remaining lines should be formatted at 72 chars.

berland commented 2 months ago

It might make sense to keep the publisher sentinel in place, to avoid rare corner cases where all events are "sent" but not yet added to the queue, and then it might exit prematurely.

Also, the commit message should be formatted to less than 50 chars on the first line, and then 72 chars for the body.