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

BUG: Fix Kwarg only in update_env #989

Merged
merged 3 commits into from Nov 6, 2023
Merged

Conversation

Carreau
Copy link
Member

@Carreau Carreau commented Oct 25, 2023

@blink1073
Copy link
Member

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
Copy link
Member

I'm reverting this change in server for now.

@Carreau
Copy link
Member Author

Carreau commented Oct 25, 2023

I pushed a check that env exists.

@blink1073
Copy link
Member

Have you tested this with server to 2.8.1?

@Carreau
Copy link
Member Author

Carreau commented Nov 6, 2023

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 Carreau marked this pull request as draft November 6, 2023 10:03
@Carreau
Copy link
Member Author

Carreau commented Nov 6, 2023

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
Copy link
Member Author

Carreau commented Nov 6, 2023

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

@Carreau Carreau marked this pull request as ready for review November 6, 2023 10:18
@@ -314,7 +320,7 @@ def format_kernel_cmd(self, extra_arguments: t.Optional[t.List[str]] = None) ->

if self.kernel_spec: # type:ignore[truthy-bool]
ns["resource_dir"] = self.kernel_spec.resource_dir

assert self._launch_args is not None
Copy link
Member Author

Choose a reason for hiding this comment

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

This helps mypy, the line below would fail anyway if it was None.

Copy link
Member

@blink1073 blink1073 Nov 6, 2023

Choose a reason for hiding this comment

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

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

You can use t.Optional["Dict[str, Any]"] to get around the runtime issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ! True, let's try

@blink1073
Copy link
Member

Papermill failure is known: nteract/papermill#737

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.

Thanks!

@blink1073 blink1073 merged commit b61accf into jupyter:main Nov 6, 2023
29 of 31 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.

Kernel error after pip install jupyterlab
2 participants