Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Monkey patch the kernel to allow debugging and tracebacks to coexist #8671

Merged
merged 6 commits into from
Jan 12, 2022

Conversation

rchiodo
Copy link
Contributor

@rchiodo rchiodo commented Jan 11, 2022

Fixes #8568

The root cause was explained in #8568 but I'll reiterate here.

We have code that does this:

import os;os.environ["IPYKERNEL_CELL_NAME"] = '${hash?.runtimeFile}'

This environment variable is used by the kernel to 'name' a cell execution so that when we get a stackframe in the debugger, it can be uniquely identified.

The problem was it was never being 'unset'.

So when an error occurs and jupyter tries to parse all of the cells that lead to an exception, it can't determine which cell <ipython-2-hashyvalue> is actually referring to.

Example:

  1. User executes def foo():\n raise Exception("FOO")\n
  2. We execute import os;os.environ["IPYKERNEL_CELL_NAME"] = '<ipython-1-hashy.py>' as a silent cell
  3. We execute def foo():\n raise Exception("FOO")\n
  4. Jupyter associates def foo():\n raise Exception("FOO")\n with <ipython-1-hashy.py>
  5. User executes foo()
  6. We execute import os;os.environ["IPYKERNEL_CELL_NAME"] = '<ipython-2-hashy2.py>' as a silent cell
  7. Jupyter associates import os;os.environ["IPYKERNEL_CELL_NAME"] = '<ipython-2-hashy2.py>' with <ipython-1-hashy.py> because IPYKERNEL_CELL_NAME was set
  8. We execute foo()
  9. Jupyter associates foo() with <ipython-2-hashy2.py>
  10. Exception is raised
  11. Traceback is generated that has the following in it
  • <ipython-1-hashy.py>
  • <ipython-2-hashy.py>
  1. <ipython-1-hashy.py> is mapped to the import os;os.environ["IPYKERNEL_CELL_NAME"] = '<ipython-2-hashy2.py> from step 7 instead of def foo():\n raise Exception("FOO")\n from step 3
  2. Traceback is invalid.

The crux of the problem is that we need an environment variable set during a cell executing but then unset for everything that comes after it (or set to something different).

What if we could execute the code we use to generate the hash inside the kernel somehow?

I had 3 ideas on how to solve this (picked the last one)

  1. Use the debugger to eval the code prior to running each cell
  2. Use the pre_run_code_hook from IPython to run code before
  3. Monkey patch the run_cell in the kernel

I picked the last one because:

  1. These entries need to be set regardless of whether or not we're debugging. It's too late if we have to wait for debugging because previous cells wouldn't have been mapped. (Makes stepping into predefined functions impossible)
  2. pre_run_code_hook is deprecated and doesn't seem to actually get the code. The code is necessary in order to compute the hash. Plus we'd have to write an entire kernel extension.

@rchiodo rchiodo requested a review from a team as a code owner January 11, 2022 19:23
@rchiodo
Copy link
Contributor Author

rchiodo commented Jan 11, 2022

@DonJayamanne it would be great if you could look at this one when you get back. I'm a little nervous patching the kernel. Thought there might be some other way to patch it.

@rchiodo
Copy link
Contributor Author

rchiodo commented Jan 11, 2022

This also fixes errors with stepping into previously defined cells. They had the same problem as the stack trace.

# Variable that prevent wrapping from happing more than_once
__VSCODE_wrapped_run_cell = False

# This function computes the hash for the code. It must follow the same algorithm as in use here:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like that logic should be in a shared location

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha nevermind, python vs ts

return codeCell && (numberOfCells === -1 || numberOfCells === codeCells!.length) ? true : false;
},
defaultNotebookTestTimeout,
`No code cell found in interactive window notebook documen`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: document,
also - is there a reason you want to use backticks for a constant string here?

@@ -593,4 +595,97 @@ def foo():
`Cursor did not move to expected line when hitting stepping into`
);
});

test('Step into a previous cell', async () => {
// Need a function and a call to i
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: another missing t!

@codecov-commenter
Copy link

Codecov Report

Merging #8671 (4e3c881) into main (9bdaf87) will decrease coverage by 0%.
The diff coverage is 92%.

❗ Current head 4e3c881 differs from pull request most recent head 981e4a6. Consider uploading reports for the commit 981e4a6 to get more accurate results

@@          Coverage Diff          @@
##            main   #8671   +/-   ##
=====================================
- Coverage     71%     71%   -1%     
=====================================
  Files        384     384           
  Lines      24624   24633    +9     
  Branches    3789    3790    +1     
=====================================
- Hits       17609   17545   -64     
- Misses      5480    5563   +83     
+ Partials    1535    1525   -10     
Impacted Files Coverage Δ
src/client/datascience/types.ts 100% <ø> (ø)
src/client/datascience/jupyter/kernels/kernel.ts 75% <85%> (+<1%) ⬆️
src/client/datascience/constants.ts 100% <100%> (ø)
...datascience/editor-integration/cellhashprovider.ts 81% <100%> (+<1%) ⬆️
...lient/datascience/jupyter/kernels/cellExecution.ts 73% <100%> (-1%) ⬇️
.../client/datascience/errors/sessionDisposedError.ts 60% <0%> (-40%) ⬇️
src/client/debugger/jupyter/debugControllers.ts 47% <0%> (-29%) ⬇️
src/client/debugger/jupyter/kernelDebugAdapter.ts 64% <0%> (-13%) ⬇️
src/client/debugger/jupyter/helper.ts 51% <0%> (-9%) ⬇️
...rc/client/datascience/jupyter/debuggerVariables.ts 58% <0%> (-8%) ⬇️
... and 8 more

@rchiodo rchiodo merged commit 8b30916 into main Jan 12, 2022
@rchiodo rchiodo deleted the rchiodo/stack_trace branch January 12, 2022 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interactive window not showing stack trace
3 participants