[python] log positron_ipykernel warnings, do not show to user
#2776
Conversation
|
(merge conflict resulting in strange errors 😩, all good now?) |
Signed-off-by: Isabel Zimmerman <54685329+isabelizimm@users.noreply.github.com>
|
|
||
| # monkey patching warning.showwarning is recommended by the official documentation | ||
| # https://docs.python.org/3/library/warnings.html#warnings.showwarning | ||
| def _showwarning(message, category, filename, lineno, file=None, line=None): |
There was a problem hiding this comment.
Wow, thanks for including this comment, I was surprised they recommend this!
Could it be more robust if we call down to the original functions with modified arguments instead of calling _showwarnmsg_impl directly?
It looks like they keep references at warnings._showwarning_orig and warnings._formatwarning_orig (source). It's slightly risky that they change this in Python 3.13 or later, but we should see a test failure and be able to fix it then so I'm not too concerned.
| # https://docs.python.org/3/library/warnings.html#warnings.showwarning | ||
| def _showwarning(message, category, filename, lineno, file=None, line=None): | ||
| # if coming from one of our files, log and don't send to user | ||
| positron_files_path = Path("python_files", "positron", "positron_ipykernel") |
There was a problem hiding this comment.
Could we get this path dynamically so it's robust to name changes? I think we could use Path(__file__).parent
extensions/positron-python/python_files/positron/positron_ipykernel/positron_ipkernel.py
Outdated
Show resolved
Hide resolved
| # vendored as-is from warnings | ||
| def _formatwarning(message, category, filename, lineno, line=None): | ||
| # --- Start Positron --- | ||
| if filename == "POSITRON_CONSOLE": |
There was a problem hiding this comment.
Rather than omitting the filename and line number for warnings from console cells, what if we displayed the cell number and line number instead? Something like:
<positron-console-cell-4>:2: UserWarning: message
We may be able to get the cell number from kernel.execution_count. But if it turns out to be too difficult, then maybe even <positron-console> is fine?
Then we also wouldn't need to override formatwarning, we could directly pass the filename through in showwarning.
For reference, here's what other shells do:
- Python:
<stdin>:2: UserWarning: message - IPython:
<ipython-input-1-b68dd3b18f8f>:2: UserWarning message - ipykernel:
/var/folders/_r/pm17dpfn7lx5mf3wtd96kw3m0000gn/T/ipykernel_49873/4243969252.py:2: UserWarning: message
(Out of curiosity, I dug into why IPython and ipykernel modify the filenames in different ways. It seems that ipykernel made the change when they implementing debugging (ipython/ipykernel#597) since they needed something that could be calculated given the cell source in both the frontend and the kernel. IIUC that file isn't actually ever created.)
There was a problem hiding this comment.
Oh that's a helpful reference for the other shells! I'll see if I can grab the cell/line, looks nice and maybe will feel more familiar for users who are expecting some sort of guidance of where the warning is coming from 👍
| @@ -308,3 +319,27 @@ def test_question_mark_help(shell: PositronShell, mock_help_service: Mock) -> No | |||
| mock_help_service.show_help.assert_called_once_with( | |||
| "positron_ipykernel.utils.positron_ipykernel_usage" | |||
| ) | |||
|
|
|||
|
|
|||
| def test_console_warning(shell: PositronShell, warning_kwargs): | |||
…ernel/positron_ipkernel.py Co-authored-by: Wasim Lorgat <mwlorgat@gmail.com> Signed-off-by: Isabel Zimmerman <54685329+isabelizimm@users.noreply.github.com>
Intent
related to #2615 #2446 (sort of #2075)
Approach
Using a custom handler for warnings. It checks if the warning is coming from
python_files/positron/positron_ipykerneland then routes it to the logs. If the error is coming from the console, do not show temp filename. You can still override this filter, eg, if you would like to raise DeprecationWarnings in the console, etc.Video of what warnings should look like ⏬
warnings.mov
todo
QA Notes
should not see temp path before warning (something like var/folders/.../47329.py)