Skip to content

Commit 1729d7d

Browse files
authored
Reticulate: Recover tests (#11232)
<!-- Thank you for submitting a pull request. If this is your first pull request you can find information about contributing here: * https://github.com/posit-dev/positron/blob/main/CONTRIBUTING.md We recommend synchronizing your branch with the latest changes in the main branch by either pulling or rebasing. --> <!-- Describe briefly what problem this pull request resolves, or what new feature it introduces. Include screenshots of any new or altered UI. Link to any GitHub issues but avoid "magic" keywords that will automatically close the issue. If there are any details about your approach that are unintuitive or you want to draw attention to, please describe them here. --> This PR overrides the default `post_handler_hook` and `pre_handler_hook` in the IPyKernel instance, so that exceptions from executing it are not raised with `exc_info=True`. Those hooks were added in ipython/ipykernel#49 to allow customizing the signal behaviors. Later, they started being executed in a try context and exceptions: ipython/ipykernel#360 Their code hasn't changed in the last 10 years, so it seems safe to override them like this. Addresses #10953 The problem we are trying to solve described in #10953 is that in Reticulate sessions on Linux, some execute requests hang, causing the kernel to be completely unresponsive. I'm not entirely sure why this fix works, but here's the hypothesis: - The kernel receives an execute_request. - The shell thread starts processing it and runs the `pre_handler_hook` which fails in reticulate (because you can't signal from the main thread) - This causes a log message to be written with `self.log.debug("Unable to signal in pre_handler_hook:", exc_info=True)` - Log messages are written to an `OutStream`, which will eventually [`flush`](https://github.com/ipython/ipykernel/blob/327589f5e0ed3759751daebb724b8f7d1a0e0c52/ipykernel/iostream.py#L587-L610). - Flushing needs to re-schedule to the IO thread and is a locking operation with a 10s timeout. This can cause hangs eg, we delay responding to rpc status messages, etc. The hypothesis is that larger tracebacks, makes the IO thread queue larger, which makes more likely that flushing times out. ### Release Notes <!-- Optionally, replace `N/A` with text to be included in the next release notes. The `N/A` bullets are ignored. If you refer to one or more Positron issues, these issues are used to collect information about the feature or bugfix, such as the relevant language pack as determined by Github labels of type `lang: `. The note will automatically be tagged with the language. These notes are typically filled by the Positron team. If you are an external contributor, you may ignore this section. --> #### New Features - N/A #### Bug Fixes - N/A ### QA Notes <!-- Positron team members: please add relevant e2e test tags, so the tests can be run when you open this pull request. - Instructions: https://github.com/posit-dev/positron/blob/main/test/e2e/README.md#pull-requests-and-test-tags - Available tags: https://github.com/posit-dev/positron/blob/main/test/e2e/infra/test-runner/test-tags.ts --> <!-- Add additional information for QA on how to validate the change, paying special attention to the level of risk, adjacent areas that could be affected by the change, and any important contextual information not present in the linked issues. --> @:reticulate @:web
1 parent 05ed75f commit 1729d7d

File tree

6 files changed

+23
-5
lines changed

6 files changed

+23
-5
lines changed

extensions/positron-python/python_files/posit/positron/positron_ipkernel.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -643,6 +643,24 @@ def _showwarning(self, message, category, filename, lineno, file=None, line=None
643643

644644
return original_showwarning(message, category, filename, lineno, file, line) # type: ignore reportAttributeAccessIssue
645645

646+
def pre_handler_hook(self):
647+
# Override the default pre_handler_hook to add debug logging.
648+
# The default logging in Ipykernel adds the exc_info=True which is causing
649+
# huge tracebacks, specially in reticulate sessions - the pre_handler_hook and
650+
# post_handler_hook always fail because they can't signal from a different thread.
651+
# See: https://github.com/posit-dev/positron/issues/10953
652+
try:
653+
super().pre_handler_hook()
654+
except Exception as e:
655+
self.log.debug("Error in super().pre_handler_hook(): %s", e, exc_info=False)
656+
657+
def post_handler_hook(self):
658+
# see the pre_handler_hook for details
659+
try:
660+
super().post_handler_hook()
661+
except Exception as e:
662+
self.log.debug("Error in super().post_handler_hook(): %s", e, exc_info=False)
663+
646664

647665
class PositronIPKernelApp(IPKernelApp):
648666
control_thread: ControlThread | None

test/e2e/tests/reticulate/reticulate-multiple.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ test.use({
1414
// RETICULATE_PYTHON
1515
// to the installed python path
1616

17-
test.describe.skip('Reticulate', {
17+
test.describe('Reticulate', {
1818
tag: [tags.RETICULATE, tags.WEB, tags.ARK, tags.SOFT_FAIL],
1919
}, () => {
2020
test.beforeAll(async function ({ app, settings }) {

test/e2e/tests/reticulate/reticulate-repl-python.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ test.use({
1414
// RETICULATE_PYTHON
1515
// to the installed python path
1616

17-
test.describe.skip('Reticulate', {
17+
test.describe('Reticulate', {
1818
tag: [tags.RETICULATE, tags.WEB, tags.ARK, tags.SOFT_FAIL],
1919
}, () => {
2020
test.beforeAll(async function ({ app, settings }) {

test/e2e/tests/reticulate/reticulate-restart.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ test.use({
1414
// RETICULATE_PYTHON
1515
// to the installed python path
1616

17-
test.describe.skip('Reticulate', {
17+
test.describe('Reticulate', {
1818
tag: [tags.RETICULATE, tags.WEB, tags.SOFT_FAIL],
1919
}, () => {
2020
test.beforeAll(async function ({ app, settings }) {

test/e2e/tests/reticulate/reticulate-stop-start.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ test.use({
1414
// RETICULATE_PYTHON
1515
// to the installed python path
1616

17-
test.describe.skip('Reticulate', {
17+
test.describe('Reticulate', {
1818
tag: [tags.RETICULATE, tags.WEB, tags.SOFT_FAIL],
1919
}, () => {
2020
test.beforeAll(async function ({ app, settings }) {

test/e2e/tests/reticulate/reticulate-variables.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ test.use({
1212
// In order to run this test on Windows, I think we need to set the env var:
1313
// RETICULATE_PYTHON to the installed python path
1414

15-
test.describe.skip('Reticulate - Variables pane support', {
15+
test.describe('Reticulate - Variables pane support', {
1616
tag: [tags.RETICULATE, tags.WEB, tags.SOFT_FAIL],
1717
}, () => {
1818
test('R - Verify Reticulate formats variables in the Variables pane', async function ({ app, sessions, logger }) {

0 commit comments

Comments
 (0)