Make sure to establish connection and empty the event queue before cancelling publisher #7562
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
berland
reviewed
Apr 4, 2024
xjules
force-pushed
the
unbox_scheduler
branch
2 times, most recently
from
5094944 to
c364398
Compare
xjules
force-pushed
the
unbox_scheduler
branch
4 times, most recently
from
30b798e to
8da5f60
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7562 +/- ##
==========================================
+ Coverage 85.19% 85.21% +0.01%
==========================================
Files 388 388
Lines 23338 23352 +14
Branches 893 887 -6
==========================================
+ Hits 19883 19899 +16
+ Misses 3348 3341 -7
- Partials 107 112 +5 ☔ View full report in Codecov by Sentry. |
jonathan-eq
reviewed
Apr 5, 2024
xjules
changed the title
9 tasks
jonathan-eq
requested review from
jonathan-eq
and removed request for
jonathan-eq
berland
reviewed
Apr 8, 2024
.pre-commit-config.yaml
Outdated
| @@ -16,7 +16,7 @@ repos: | |||
| - id: ruff-format | |||
|
|
|||
| - repo: https://github.com/pre-commit/mirrors-clang-format | |||
| rev: v18.1.1 | |||
| rev: v18.1.3 | |||
There was a problem hiding this comment.
would be nice to have in a separate commit in case of rollbacks.
There was a problem hiding this comment.
Sorry, left over. Will rm.
berland
reviewed
Apr 8, 2024
src/ert/async_utils.py
Outdated
| @@ -40,6 +41,16 @@ def _done_callback(task: asyncio.Task[_T_co]) -> None: | |||
| if (exc := task.exception()) is None: | |||
| return | |||
|
|
|||
| logger.error(f"Exception occurred during {task.get_name()}", exc_info=exc) | |||
| # logger.error(f"Exception occurred during {task.get_name()}", exc_info=exc) | |||
berland
reviewed
Apr 8, 2024
| @@ -164,7 +164,6 @@ def sample_prior(nx, ny): | |||
| ) | |||
|
|
|||
|
|
|||
| @pytest.mark.skip(reason="Very flaky with scheduler") | |||
There was a problem hiding this comment.
thank you for not forgetting this! 😄
berland
reviewed
Apr 8, 2024
| @@ -215,14 +225,18 @@ async def _monitor_and_handle_tasks( | |||
| ) | |||
| logger.error( | |||
| ( | |||
| f"Exception in scheduler task: {task_exception}\n" | |||
| f"Exception in scheduler task {task.get_name()}: {task_exception}\n" | |||
There was a problem hiding this comment.
This should perhaps be a separate commit, as it is unrelated to fixing the premature closing of publisher.
There was a problem hiding this comment.
Now it's in two commits. 1) improving exception messages 2) the actual fix
xjules
force-pushed
the
unbox_scheduler
branch
2 times, most recently
from
6f85d2d to
ab1bf8f
Compare
- Add _publisher_done event and CLOSE_PUBLISHER_SENTINEL to make sure that the connection was established and all events were sent before the cancellation happens. - Supress CancelledError when task gets cancelled for long running jobs - Ignore cancellation in job task - Add test for scheduler publishings its events to a websocket with publisher_done set Event. Co-authored-by: Håvard Berland <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
Resolves #7522
When applicable