jupyter / jupyter_client

Jupyter protocol client APIs
https://jupyter-client.readthedocs.io
BSD 3-Clause "New" or "Revised" License
374 stars 279 forks source link

BUG: Fix Kwarg only in update_env #989

Closed Carreau closed 8 months ago

Carreau commented 9 months ago

closes jupyterlab/jupyterlab#15301

blink1073 commented 9 months ago

Looks like there's still an issue with the downstream server tests.

   Traceback (most recent call last):
    File "/home/runner/test_venv/lib/python3.11/site-packages/tornado/web.py", line 1786, in _execute
      result = await result
               ^^^^^^^^^^^^
    File "/home/runner/test_venv/downstream_test/jupyter_server/jupyter_server/services/sessions/handlers.py", line 172, in patch
      await sm.update_session(session_id, **changes)
    File "/home/runner/test_venv/downstream_test/jupyter_server/jupyter_server/services/sessions/sessionmanager.py", line 469, in update_session
      self.kernel_manager.update_env(kernel_id=kernel_id, env=self.get_kernel_env(path, name))
    File "/home/runner/test_venv/lib/python3.11/site-packages/jupyter_client/multikernelmanager.py", line 226, in update_env
      self._kernels[kernel_id].update_env(env=env)
    File "/home/runner/test_venv/lib/python3.11/site-packages/jupyter_client/manager.py", line 284, in update_env
      self._launch_args['env'].update(env)
      ~~~~~~~~~~~~~~~~~^^^^^^^
  KeyError: 'env'
blink1073 commented 9 months ago

I'm reverting this change in server for now.

Carreau commented 9 months ago

I pushed a check that env exists.

blink1073 commented 9 months ago

Have you tested this with server to 2.8.1?

Carreau commented 8 months ago

Ok, I've been more agressive on the condition, checking that everything exists and are dicts, and added all the corresponding test cases.

I've tested on Jupyter_server 2.7.0, 2.9.1 (does not crash, but does nothing), and on 2.9.0 (it does work an update __session__).

Carreau commented 8 months ago

Let me see if I can fix the typing of _launch_args, that is set to Any, I think we can make it t.Optional[Dict]

Carreau commented 8 months ago

ok, I can't make it t.Optional[Dict[str, Any]], as Dict is not subscriptable, but I did make it t.Optional[Dict]

blink1073 commented 8 months ago

Papermill failure is known: https://github.com/nteract/papermill/issues/737