fix: Monkeypatch BigQuery library so KeyboardInterrupt cancels active query #34
Conversation
📝 WalkthroughWalkthroughDuring Deepnote runtime initialization, Sequence Diagram(s)sequenceDiagram
participant Init as Deepnote Runtime Init
participant RuntimePatches as runtime_patches.apply_runtime_patches
participant BigQueryHelpers as google.cloud.bigquery._job_helpers
participant Job as BigQuery Job
participant Tests as Unit Tests (autouse fixture)
Init->>RuntimePatches: call apply_runtime_patches()
RuntimePatches->>BigQueryHelpers: attempt monkeypatch _wait_or_cancel
Note right of RuntimePatches: log success or warning on failure
Tests->>RuntimePatches: ensure patches applied (autouse)
Tests->>Job: invoke job.result() (via _wait_or_cancel)
activate Job
Job-->>Job: raise KeyboardInterrupt / exception
Job->>Job: patched _wait_or_cancel catches exception
Job->>Job: call job.cancel(retry=None, timeout=api_timeout)
Job-->>Tests: re-raise KeyboardInterrupt
deactivate Job
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/unit/conftest.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (1)
Comment |
|
📦 Python package built successfully!
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #34 +/- ##
=======================================
Coverage 72.88% 72.89%
=======================================
Files 93 94 +1
Lines 5142 5168 +26
Branches 754 754
=======================================
+ Hits 3748 3767 +19
- Misses 1150 1157 +7
Partials 244 244
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
deepnote_toolkit/sql/sql_execution.py(3 hunks)tests/unit/test_sql_execution_internal.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/test_sql_execution_internal.py (1)
deepnote_toolkit/sql/sql_execution.py (1)
_wait_or_cancel(53-73)
🪛 Ruff (0.14.5)
deepnote_toolkit/sql/sql_execution.py
46-46: Missing return type annotation for private function _monkeypatch_bigquery_wait_or_cancel
Add return type annotation: None
(ANN202)
57-57: Dynamically typed expressions (typing.Any) are disallowed in retry
(ANN401)
71-72: try-except-pass detected, consider logging the exception
(S110)
71-71: Do not catch blind exception: Exception
(BLE001)
79-79: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test - Python 3.11
- GitHub Check: Test - Python 3.10
- GitHub Check: Test - Python 3.12
- GitHub Check: Test - Python 3.13
- GitHub Check: Test - Python 3.9
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.11
🔇 Additional comments (4)
tests/unit/test_sql_execution_internal.py (1)
12-31: LGTM – Test correctly validates KeyboardInterrupt handling.The test properly verifies that KeyboardInterrupt propagates and that job cancellation occurs with the expected parameters.
deepnote_toolkit/sql/sql_execution.py (3)
7-7: LGTM – Imports support logging and typing.Also applies to: 28-28, 38-38
76-83: LGTM – Module-level invocation with appropriate error handling.Applying the monkeypatch at import time ensures it's active before BigQuery usage. Error handling gracefully degrades when the library isn't available.
70-70: Cannot verify upstream PR reference; pattern appears intentional.PR 2331 reference couldn't be located in public repositories. However, the design of passing
retrytocancel()is defensible: it maintains consistency with the function'sretryparameter and aligns with Google Cloud client patterns. The trade-off (potentially slower cancellation with aggressive retry settings) is worth documenting if not already covered in docstrings.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
deepnote_toolkit/sql/sql_execution.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.14.5)
deepnote_toolkit/sql/sql_execution.py
57-57: Dynamically typed expressions (typing.Any) are disallowed in retry
(ANN401)
71-72: try-except-pass detected, consider logging the exception
(S110)
71-71: Do not catch blind exception: Exception
(BLE001)
83-83: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Combine and Upload Coverage
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Build and push artifacts for Python 3.11
🔇 Additional comments (2)
deepnote_toolkit/sql/sql_execution.py (2)
7-7: LGTM: Clean import additions.Imports support the monkeypatch logging and type annotations.
Also applies to: 28-28, 38-38
87-87: Appropriate module-level execution.Applying the monkeypatch at import time ensures it's active before any BigQuery operations. The comprehensive error handling in the function prevents import failures.
|
🚀 Review App Deployment Started
|
…does-not-cancel' of github.com:deepnote/deepnote-toolkit into oleh/blu-5131-moon-active-cancelling-query-in-deepnote-does-not-cancel
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deepnote_toolkit/runtime_initialization.py (1)
9-33: Tighten logging and linter story forapply_runtime_patchesfailure pathCatching
Exceptionhere is reasonable (patch failure shouldn’t break runtime), but it’s worth making this explicit and improving diagnostics.You could both satisfy the linter and get a traceback:
logger.debug("Initializing Deepnote runtime environment started.") - try: - apply_runtime_patches() - except Exception as e: - logger.error("Failed to apply runtime patches with a error: %s", e) + try: + apply_runtime_patches() + except Exception: # pylint: disable=broad-exception-caught # noqa: BLE001 + logger.exception("Failed to apply runtime patches with error")This matches the rest of the file’s defensive style while giving you stack traces when the monkeypatching goes wrong.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
deepnote_toolkit/runtime_initialization.py(2 hunks)deepnote_toolkit/runtime_patches.py(1 hunks)tests/unit/conftest.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
deepnote_toolkit/runtime_initialization.py (2)
deepnote_toolkit/runtime_patches.py (1)
apply_runtime_patches(52-53)tests/unit/conftest.py (1)
apply_runtime_patches(11-15)
tests/unit/conftest.py (1)
deepnote_toolkit/runtime_patches.py (1)
apply_runtime_patches(52-53)
deepnote_toolkit/runtime_patches.py (1)
tests/unit/conftest.py (1)
apply_runtime_patches(11-15)
🪛 Ruff (0.14.5)
deepnote_toolkit/runtime_initialization.py
31-31: Do not catch blind exception: Exception
(BLE001)
32-32: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
deepnote_toolkit/runtime_patches.py
13-13: Missing return type annotation for private function _monkeypatch_bigquery_wait_or_cancel
Add return type annotation: None
(ANN202)
22-22: Dynamically typed expressions (typing.Any) are disallowed in retry
(ANN401)
36-37: try-except-pass detected, consider logging the exception
(S110)
36-36: Do not catch blind exception: Exception
(BLE001)
48-48: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Test - Python 3.9
- GitHub Check: Test - Python 3.10
- GitHub Check: Test - Python 3.11
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Build and push artifacts for Python 3.12
🔇 Additional comments (1)
deepnote_toolkit/runtime_patches.py (1)
52-53: Clear single entrypoint for runtime patchesHaving
apply_runtime_patches()as the only public entry that delegates to private helpers keeps the surface area small and makes future patches easy to add.
OlegWock
deleted the
oleh/blu-5131-moon-active-cancelling-query-in-deepnote-does-not-cancel
branch
You can test this by executing slow query, e.g. this one (takes around 30-40 seconds for me). Then stopping block execution and checking BigQuery "Jobs explorer" (in GCP console) that job was created and cancelled (if you cancel too quickly after start of execution it's possible job won't be created at all, so to test it I'd recommend to wait 10s before cancelling block execution).
Summary by CodeRabbit
New Features
Tests
Other