query: interrupt query script on SIGTERM #858

skshetry · 2025-01-24T12:50:18Z

This now supports gracefully terminating script on SIGTERM. Graceful shutdown is also done on Ctrl-C.

Also added logs.

codecov · 2025-01-24T12:55:57Z

Codecov Report

Attention: Patch coverage is 96.07843% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.60%. Comparing base (ef23a20) to head (fbab6bb).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
src/datachain/catalog/catalog.py	96.07%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #858      +/-   ##
==========================================
- Coverage   87.61%   87.60%   -0.02%     
==========================================
  Files         128      128              
  Lines       11385    11431      +46     
  Branches     1540     1543       +3     
==========================================
+ Hits         9975    10014      +39     
- Misses       1023     1028       +5     
- Partials      387      389       +2

Flag	Coverage Δ
datachain	`87.52% <94.11%> (-0.02%)`	⬇️

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.

cloudflare-workers-and-pages · 2025-01-24T12:58:23Z

Deploying datachain-documentation with Cloudflare Pages

Latest commit:	`fbab6bb`
Status:	✅ Deploy successful!
Preview URL:	https://1bcd9044.datachain-documentation.pages.dev
Branch Preview URL:	https://handle-sigterm.datachain-documentation.pages.dev

View logs

shcheklein · 2025-01-25T14:24:01Z

Is there a way to test this?

src/datachain/catalog/catalog.py

skshetry · 2025-01-27T11:36:42Z

Is there a way to test this?

I don't see an easy way to test this unfortunately. I don't want to write some heavy tests, and simple mock tests are not going to be useful.

src/datachain/catalog/catalog.py

This now supports gracefully terminating script on SIGTERM. Also added logs.

skshetry · 2025-01-27T14:02:04Z

src/datachain/catalog/catalog.py

+            # ignore SIGINT in the main process
+            signal.signal(signal.SIGINT, signal.SIG_IGN)


We need two different behaviors from this query API that this has become complicated.

CLI should ideally ignore SIGINT and forward SIGTERM to the child.
However, Celery tasks should not ignore SIGINT, but consider SIGTERM as a special signal for graceful termination.

Here, I have made a compromise: to ignore SIGINT and use SIGTERM as a special signal for graceful termination everywhere.

So, SIGINT still works for CLI (where it matters most), and SIGTERM will still work for canceling celery tasks.

so, I'm a bit lost - are we ignoring SIGINT in CLI or not?

We are ignoring Ctrl-C in the main process. If you are running in terminal, OS (tty driver) will send the signal to all processes in the foreground process group, so the script will also get the signal. If we forward signals, then the subprocess will receive the signal twice.

If you are running a background process (e.g.: via a script), you'd have to interrupt the script subprocess rather than the CLI process.

will send the signal to all processes in the foreground process group

okay, I see

I think it would be helpful to describe these details in the comment for the next generations, it's a dense, low level code

mattseddon

Looks good

mattseddon · 2025-01-27T21:41:46Z

src/datachain/catalog/catalog.py

+                signal.signal(signal.SIGTERM, orig_sigterm_handler)
+                signal.signal(signal.SIGINT, orig_sigint_handler)
+                if thread:
+                    thread.join()  # wait for the reader thread


[Q] Will this still run into deadlocks while we are waiting for tqdm/tqdm#1649?

Deadlock was happening when fp.write() was raising exception. That won't happen now that we use SIGTERM to cancel and wait.

But I can imagine, when the conditions are favorable, the tqdm may still deadlock.
But the chances are slim. Eg: when you press Ctrl-C right at the time display runs, it may deadlock if there are other tqdm processes running in other thread.

mattseddon · 2025-01-27T21:43:21Z

Is there a way to test this?

Unfortunately, we need an e2e test in Studio. I can help with this.

dreadatour

Looks good to me.

shcheklein · 2025-01-28T01:52:22Z

src/datachain/catalog/catalog.py

+    SIGINT = signal.SIGINT
+
+
+def shutdown_process(


unit test this?

There's no point in unittesting this. We'll check mock_popen.terminate.assert_called_once() which is not that useful.

I mean function test this - it should not be about testing "mocks" or course

shcheklein · 2025-01-28T02:06:04Z

src/datachain/catalog/catalog.py

+            # ignore SIGINT in the main process
+            signal.signal(signal.SIGINT, signal.SIG_IGN)
+
+            orig_sigterm_handler = signal.getsignal(signal.SIGTERM)


this should be happening inside try (to avoid getting the TerminationSignal raised in between)

TerminationSignal is raised only after we set a signal handler, which happens after this line.

so it can be raised right before try?

Theoretically yes, it could.

so, then we should move it inside and catch and make sure that we handle if for example of the original handlers is not yet initialized, etc

this a few lines code change, no?

also, doing multithreading / multiprocessing is always hard exactly because lot of problems manifest themselves rarely but bite hard in terms of time someone can then spend debugging them later ... so, yes, if there are obvious improvements while it is still possible w/o obvious downsides I would always err on the side of doing safer code

The best thing to do here is move sigterm handler out of this with loop.

also, doing multithreading / multiprocessing is always hard exactly because lot of problems manifest themselves rarely but bite hard in terms of time someone can then spend debugging them later ... so, yes, if there are obvious improvements while it is still possible w/o obvious downsides I would always err on the side of doing safer code

There is no multithreading or multiprocessing involved here. The main thread always gets a signal.

The best thing to do here is move sigterm handler out of this with loop.

probably, and in except clause and in finally check if proc is setup, if original handlers are set

There is no multithreading or multiprocessing involved here. The main thread always gets a signal.

there is to my mind. An external process sends a signal to a another one.

src/datachain/catalog/catalog.py

shcheklein · 2025-01-28T02:09:30Z

tests/unit/test_query.py

@@ -63,3 +66,15 @@ def test_non_zero_exitcode(catalog, mock_popen):
        catalog.query("pass")
    assert e.value.return_code == 1
    assert "Query script exited with error code 1" in str(e.value)
+
+
+def test_shutdown_process_on_sigterm(mocker, catalog, mock_popen):


not sure tbh this is a very meaningful test :) seems we are testing mocks primarily

The main goal of this test here is to assert that shutdown_process was called (and with expected values).

yep, I understand. Doesn't see to be very useful though. Or let me put it in a different way - I think we rarely test things this way (to check if function is called w/o testing logic in some way). Still seems to be not very meaningful to my mind. While we can do in this case most likely a proper test

Handling signals is a tricky thing, especially when the main process needs to receive it (where it happens to be a test process for tests).

I think we rarely test things this way (to check if function is called w/o testing logic in some way

We do that in plenty of places in DVC, especially in CLI tests.

While we can do in this case most likely a proper test

If you have some example unittest that we can replace this with (without crossing boundaries into the working of shutdown_process), I'd be very interested.
Again, my goal with this test is to make sure that the shutdown_process gets called with expected values. If you see a way to do that, I'll be happy to write it, or review it.

I did add a functional test, but it's complex, and you have to tread very carefully not to get main process killed/blocked/waiting forever.

skshetry · 2025-01-28T10:31:58Z

Unfortunately, we need an e2e test in Studio. I can help with this.

I have added a functional test for SIGTERM shutdown. It looks very complicated, and I don't see any simpler way to test this. Open to suggestions.

skshetry force-pushed the handle-sigterm branch from 36fb854 to ff93db2 Compare January 24, 2025 12:58

skshetry force-pushed the handle-sigterm branch from ff93db2 to 68e5cfd Compare January 24, 2025 13:27

shcheklein reviewed Jan 25, 2025

View reviewed changes

src/datachain/catalog/catalog.py Outdated Show resolved Hide resolved

skshetry force-pushed the handle-sigterm branch from 68e5cfd to d5c10f6 Compare January 27, 2025 11:49

skshetry requested a review from a team January 27, 2025 12:11

skshetry commented Jan 27, 2025

View reviewed changes

src/datachain/catalog/catalog.py Outdated Show resolved Hide resolved

skshetry force-pushed the handle-sigterm branch 4 times, most recently from a50bc43 to 9525334 Compare January 27, 2025 13:48

query: interrupt query script gracefully on SIGTERM

89b9323

This now supports gracefully terminating script on SIGTERM. Also added logs.

skshetry force-pushed the handle-sigterm branch from 9525334 to 89b9323 Compare January 27, 2025 13:53

skshetry commented Jan 27, 2025

View reviewed changes

skshetry requested review from shcheklein and dreadatour January 27, 2025 17:01

mattseddon approved these changes Jan 27, 2025

View reviewed changes

dreadatour approved these changes Jan 28, 2025

View reviewed changes

shcheklein reviewed Jan 28, 2025

View reviewed changes

src/datachain/catalog/catalog.py Show resolved Hide resolved

shcheklein reviewed Jan 28, 2025

View reviewed changes

skshetry force-pushed the handle-sigterm branch 3 times, most recently from f7e7723 to bd4d98a Compare January 28, 2025 10:30

add functional test

fbab6bb

skshetry force-pushed the handle-sigterm branch from bd4d98a to fbab6bb Compare January 28, 2025 10:37

skshetry merged commit c0f23b4 into main Jan 28, 2025
37 checks passed

skshetry deleted the handle-sigterm branch January 28, 2025 10:57

skshetry mentioned this pull request Jan 31, 2025

move signal handler for sigterm #873

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

query: interrupt query script on SIGTERM #858

query: interrupt query script on SIGTERM #858

skshetry commented Jan 24, 2025

codecov bot commented Jan 24, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Jan 24, 2025 •

edited

Loading

shcheklein commented Jan 25, 2025

skshetry commented Jan 27, 2025 •

edited

Loading

skshetry Jan 27, 2025 •

edited

Loading

shcheklein Jan 28, 2025

skshetry Jan 28, 2025

shcheklein Jan 28, 2025

mattseddon left a comment

mattseddon Jan 27, 2025

skshetry Jan 28, 2025

mattseddon commented Jan 27, 2025

dreadatour left a comment

shcheklein Jan 28, 2025

skshetry Jan 28, 2025

shcheklein Jan 28, 2025

shcheklein Jan 28, 2025

skshetry Jan 28, 2025

shcheklein Jan 28, 2025

skshetry Jan 28, 2025

shcheklein Jan 28, 2025

shcheklein Jan 29, 2025

skshetry Jan 29, 2025

skshetry Jan 29, 2025

shcheklein Jan 29, 2025

skshetry Jan 29, 2025

shcheklein Jan 28, 2025

skshetry Jan 28, 2025 •

edited

Loading

shcheklein Jan 28, 2025

skshetry Jan 29, 2025 •

edited

Loading

skshetry commented Jan 28, 2025

		# ignore SIGINT in the main process
		signal.signal(signal.SIGINT, signal.SIG_IGN)

query: interrupt query script on SIGTERM #858

query: interrupt query script on SIGTERM #858

Conversation

skshetry commented Jan 24, 2025

codecov bot commented Jan 24, 2025 • edited Loading

Codecov Report

cloudflare-workers-and-pages bot commented Jan 24, 2025 • edited Loading

Deploying datachain-documentation with Cloudflare Pages

shcheklein commented Jan 25, 2025

skshetry commented Jan 27, 2025 • edited Loading

skshetry Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattseddon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattseddon commented Jan 27, 2025

dreadatour left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skshetry Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skshetry Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

skshetry commented Jan 28, 2025

codecov bot commented Jan 24, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Jan 24, 2025 •

edited

Loading

skshetry commented Jan 27, 2025 •

edited

Loading

skshetry Jan 27, 2025 •

edited

Loading

skshetry Jan 28, 2025 •

edited

Loading

skshetry Jan 29, 2025 •

edited

Loading