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

fix: on exception, return a 0, so that the "sum" still computes #1204

Merged

Conversation

petervandenabeele
Copy link
Contributor

@petervandenabeele petervandenabeele commented Feb 5, 2024

This fixes a bug where after a while in a Jupyter notebook, we start to see many log lines like this:

[IPKernelApp] ERROR | Exception in control handler:
Traceback (most recent call last):
  File ".../lib/python3.8/site-packages/ipykernel/kernelbase.py", line 336, in process_control
    await result
  File ".../lib/python3.8/site-packages/ipykernel/kernelbase.py", line 998, in usage_request
    reply_content["kernel_memory"] = sum(
TypeError: unsupported operand type(s) for +: 'int' and 'NoneType'

Since the "sum" expects a number (float or int) as elements given to the "sum", we should pass "0" (zero) and not None when that process id does not exist anymore.

This was tested in an actually running notebook and it fixed the problem:

  • no more Exceptions like the one reported above
  • correct memory stats where seen with the Kernel Usage plug-ins

This fixes a bug where after a while in a Jupyter notebook, we start
to see many log lines like this:

```
[IPKernelApp] ERROR | Exception in control handler:
Traceback (most recent call last):
  File ".../lib/python3.8/site-packages/ipykernel/kernelbase.py", line 336, in process_control
    await result
  File ".../lib/python3.8/site-packages/ipykernel/kernelbase.py", line 998, in usage_request
    reply_content["kernel_memory"] = sum(
TypeError: unsupported operand type(s) for +: 'int' and 'NoneType'
```

Since the "sum" expects a number (float or int) as elements given to the "sum",
we should pass "0" (zero) and not None when we that process id does not exist
anymore.

This was tested in an actually running notebook and it fixed the problem:
* no morre Exceptions like the one reported above
* correct memory stats where seen with the Kernel Usage plug-ins
@petervandenabeele
Copy link
Contributor Author

This is a bug fix and needs the "bugfix" Label. As an external contributor, I am unable to set it.

Also, I am pinging @echarles since we discussed this out-of-band and Eric is marked in git as the previous contributor that touched this code.

Many thanks for your attention (I hacked it locally here in the site-packages and it seems to work for me :-)

Copy link
Member

@echarles echarles left a comment

Choose a reason for hiding this comment

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

@echarles echarles enabled auto-merge (squash) February 5, 2024 14:36
@echarles echarles added the bug label Feb 5, 2024
@echarles
Copy link
Member

echarles commented Feb 5, 2024

@blink1073 📝 test_lint → ❌ failure [required to succeed] is failing, not sure about that one.

@petervandenabeele
Copy link
Contributor Author

There seems to be a problem with a linting ignore that is not correctly set-up ?

https://github.com/ipython/ipykernel/actions/runs/7785980351/job/21229753213?pr=1204#step:4:75

ipykernel/kernelapp.py:678: error: Unused "type: ignore" comment 
[unused-ignore]
                pdb.set_trace = debugger.set_trace  # type:ignore[assignme...
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Installing missing stub packages:
/home/runner/.cache/pre-commit/repo3ioeb974/py_env-python3.11/bin/python -m pip install types-Pygments types-colorama types-decorator types-psutil types-pycurl types-python-dateutil types-six


Found 1 error in 1 file (checked 43 source files)

Error: Process completed with exit code 1.

@petervandenabeele
Copy link
Contributor Author

FYI: the same "linting" problem also occurs in another PR I just made today (5 Feb 2024) and that only added 1 line to .gitignore:

#1203

@blink1073
Copy link
Member

The lint failures comes from a version update in the linter, I'll address in a separate PR.

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thank you!

@blink1073 blink1073 merged commit a993626 into ipython:main Feb 6, 2024
30 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants