[Core] Defer SIGINT interrupt during task argument deserialization. #30476

clarkzinzow · 2022-11-18T17:55:21Z

Importing certain libraries (e.g. Arrow, Pandas, Torch) is not reentrant, and task cancellation is occasionally interrupting the Arrow import triggered via this deserialization add-on during task argument deserialization, which we are then trying to import again when serializing the error. See here for an example failure: https://buildkite.com/ray-project/oss-ci-build-branch/builds/1115#018485e1-df32-480f-9c36-cc898341f0a2

This PR prevents this import reentrancy from happening for the task cancellation case by deferring interrupts until after task argument deserialization finishes, so we can be sure that the serialization-related imports have finished before processing the interrupt.

Related issue number

Closes #30453

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

python/ray/_raylet.pyx

clarkzinzow · 2022-11-18T18:08:53Z

python/ray/tests/test_cancel.py

+
+    ray._private.worker.global_worker.run_function_on_all_workers(
+        register_non_reentrant_import_and_delegate_reducer,
+    )


@iycheng Using run_function_on_all_workers was the only way I could think of to make this test clean, lmk if you have any other ideas using runtime environment plugins and the like!

python/ray/_private/utils.py

clarkzinzow · 2022-11-22T00:11:57Z

@stephanie-wang @rickyyx @rkooo567 Ping for review!

…text.

… support Python 3.6.

scv119 · 2022-11-22T18:54:55Z

python/ray/_raylet.pyx

+                        # See https://github.com/ray-project/ray/issues/30453.
+                        # NOTE (Clark): Signal handlers can only be registered on the
+                        # main thread.
+                        with DeferSigint.create_if_main_thread():


should hijack this in ray core, or only in the arrow serialization plugin?

This is a generic problem for Ray Core, so I don't think that we'd want to only fix it for the arrow serialization plugin. Other serialization add-ons and dependencies that we import on import ray (e.g. NumPy) may suffer from non-reentrant imports either now or in the future, so if it's easy to solve this generically, I think that we should.

scv119 · 2022-11-22T18:56:23Z

the fix looks simpler than I thought!

scv119 · 2022-11-22T19:07:39Z

python/ray/_private/utils.py

+        # Monkey patch signal.signal to raise an error if a SIGINT handler is registered
+        # within the context.
+        self.orig_signal = signal.signal
+        signal.signal = self._signal_monkey_patch


how reliable is this monkey patch?
think about it again, an alternative is to defer sending signals when serialization is in progress:
https://sourcegraph.com/github.com/ray-project/ray/-/blob/python/ray/_raylet.pyx?L1210

This monkey patch should be pretty reliable unless signal is manually reloaded with importlib in the same process.

I should stress that this isn't technically needed for our current use of the context manager on task argument deserialization, this is just to:

return a nice error to users that are doing something really weird, like registering a signal handler in the deserialization-side of a user-defined type pickle reducer,

guard against future uses of this context manager elsewhere in Ray Core.

think about it again, an alternative is to defer sending signals when serialization is in progress:
https://sourcegraph.com/github.com/ray-project/ray/-/blob/python/ray/_raylet.pyx?L1210

Yeah I thought of that, but I thought that deferring the signal to the end of the code block in question would be cleaner than adding another mutex-guarded is_deserializing_args flag and relying on the task cancellation RPC retry.

In my testing, I think that I hit some hangs and silent failures when task cancellation was received before the task ID was set (i.e. with the RPC retry being hit), so I'm worried that there are some undiscovered bugs there. Going to dig a bit further into that to see if I can get a reproduction.

python/ray/tests/test_cancel.py

…o support Windows.

scv119 · 2022-11-24T00:48:23Z

failures look related?

clarkzinzow · 2022-11-24T17:40:17Z

@scv119 Hmm weird, I would have expected that flaky Datasets test to be fixed by this PR, but maybe task cancellation is still hitting a code section that isn't properly handling the interrupt...

clarkzinzow · 2022-11-29T22:56:29Z

Datasets build failure determined to be an unrelated task cancellation bug that will be fixed in another PR, and other test failures are confirmed to be unrelated already-flaky tests, so I'm going to merge this.

…ay-project#30476) Importing certain libraries (e.g. Arrow, Pandas, Torch) is not reentrant, and task cancellation is occasionally interrupting the Arrow import triggered via this deserialization add-on during task argument deserialization, which we are then trying to import again when serializing the error. See here for an example failure: https://buildkite.com/ray-project/oss-ci-build-branch/builds/1115#018485e1-df32-480f-9c36-cc898341f0a2 This PR prevents this import reentrancy from happening for the task cancellation case by deferring interrupts until after task argument deserialization finishes, so we can be sure that the serialization-related imports have finished before processing the interrupt.

…ay-project#30476) Importing certain libraries (e.g. Arrow, Pandas, Torch) is not reentrant, and task cancellation is occasionally interrupting the Arrow import triggered via this deserialization add-on during task argument deserialization, which we are then trying to import again when serializing the error. See here for an example failure: https://buildkite.com/ray-project/oss-ci-build-branch/builds/1115#018485e1-df32-480f-9c36-cc898341f0a2 This PR prevents this import reentrancy from happening for the task cancellation case by deferring interrupts until after task argument deserialization finishes, so we can be sure that the serialization-related imports have finished before processing the interrupt. Signed-off-by: Weichen Xu <[email protected]>

…ay-project#30476) Importing certain libraries (e.g. Arrow, Pandas, Torch) is not reentrant, and task cancellation is occasionally interrupting the Arrow import triggered via this deserialization add-on during task argument deserialization, which we are then trying to import again when serializing the error. See here for an example failure: https://buildkite.com/ray-project/oss-ci-build-branch/builds/1115#018485e1-df32-480f-9c36-cc898341f0a2 This PR prevents this import reentrancy from happening for the task cancellation case by deferring interrupts until after task argument deserialization finishes, so we can be sure that the serialization-related imports have finished before processing the interrupt. Signed-off-by: tmynn <[email protected]>

clarkzinzow assigned stephanie-wang, rickyyx, rkooo567 and fishbone Nov 18, 2022

clarkzinzow force-pushed the core/fix/defer-sigint-task-arg-deser branch from 779a088 to 92ce39c Compare November 18, 2022 18:02

clarkzinzow commented Nov 18, 2022

View reviewed changes

python/ray/_raylet.pyx Outdated Show resolved Hide resolved

clarkzinzow commented Nov 18, 2022

View reviewed changes

cadedaniel reviewed Nov 18, 2022

View reviewed changes

python/ray/_private/utils.py Show resolved Hide resolved

clarkzinzow added 2 commits November 22, 2022 15:09

Defer SIGINT interrupt during task argument deserialization.

7a0847d

Limit SIGINT deferment to tasks, only defer if in the main thread.

53bf1aa

clarkzinzow force-pushed the core/fix/defer-sigint-task-arg-deser branch from 3d8f8d8 to 53bf1aa Compare November 22, 2022 17:04

clarkzinzow added 2 commits November 22, 2022 17:20

Raise an error if SIGINT handler is registered within DeferSigint con…

d028ade

…text.

Remove Torch dependency from cancellation test.

b7dcbdc

clarkzinzow requested a review from cadedaniel November 22, 2022 17:26

Use contextlib.suppress instead of contextlib.nullcontext in order to…

f527f2c

… support Python 3.6.

clarkzinzow assigned scv119 Nov 22, 2022

scv119 reviewed Nov 22, 2022

View reviewed changes

clarkzinzow mentioned this pull request Nov 22, 2022

[Core][Data] fix circular import for ray.data in AsyncActors #30558

Merged

7 tasks

rickyyx reviewed Nov 22, 2022

View reviewed changes

python/ray/tests/test_cancel.py Show resolved Hide resolved

scv119 approved these changes Nov 22, 2022

View reviewed changes

scv119 added the v2.2.0-pick label Nov 22, 2022

scv119 reviewed Nov 22, 2022

View reviewed changes

python/ray/tests/test_cancel.py Show resolved Hide resolved

Use _thread.interrupt_main() instead of os.kill() in tests in order t…

faabfbd

…o support Windows.

scv119 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 24, 2022

clarkzinzow mentioned this pull request Nov 24, 2022

[ci] test_dataset is failing on release branch #30644

Closed

xwjiang2010 added the release-blocker P0 Issue that blocks the release label Nov 29, 2022

clarkzinzow merged commit 4872416 into ray-project:master Nov 29, 2022

clarkzinzow mentioned this pull request Nov 29, 2022

[Cherry-pick] [Core] Defer SIGINT interrupt during task argument deserialization. (#30476) #30754

Closed

scv119 mentioned this pull request May 3, 2023

[Core][Release test] microbenchmark_38.aws failed #34915

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Defer SIGINT interrupt during task argument deserialization. #30476

[Core] Defer SIGINT interrupt during task argument deserialization. #30476

clarkzinzow commented Nov 18, 2022

clarkzinzow Nov 18, 2022 •

edited

Loading

clarkzinzow commented Nov 22, 2022

scv119 Nov 22, 2022

clarkzinzow Nov 22, 2022 •

edited

Loading

scv119 commented Nov 22, 2022

scv119 Nov 22, 2022

clarkzinzow Nov 22, 2022 •

edited

Loading

clarkzinzow Nov 22, 2022

scv119 commented Nov 24, 2022

clarkzinzow commented Nov 24, 2022

clarkzinzow commented Nov 29, 2022

[Core] Defer SIGINT interrupt during task argument deserialization. #30476

[Core] Defer SIGINT interrupt during task argument deserialization. #30476

Conversation

clarkzinzow commented Nov 18, 2022

Related issue number

Checks

clarkzinzow Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

clarkzinzow commented Nov 22, 2022

scv119 Nov 22, 2022

Choose a reason for hiding this comment

clarkzinzow Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

scv119 commented Nov 22, 2022

scv119 Nov 22, 2022

Choose a reason for hiding this comment

clarkzinzow Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

clarkzinzow Nov 22, 2022

Choose a reason for hiding this comment

scv119 commented Nov 24, 2022

clarkzinzow commented Nov 24, 2022

clarkzinzow commented Nov 29, 2022

clarkzinzow Nov 18, 2022 •

edited

Loading

clarkzinzow Nov 22, 2022 •

edited

Loading

clarkzinzow Nov 22, 2022 •

edited

Loading