-
Notifications
You must be signed in to change notification settings - Fork 310
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
Use kernel_id for new kernel if it doesn't exist in MappingKernelManager.start_kernel #511
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
Codecov Report
@@ Coverage Diff @@
## master #511 +/- ##
==========================================
- Coverage 77.12% 77.07% -0.06%
==========================================
Files 107 107
Lines 9490 9491 +1
Branches 1026 1027 +1
==========================================
- Hits 7319 7315 -4
- Misses 1804 1807 +3
- Partials 367 369 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Great stuff!
@kevin-bates or @vidartf, do you mind taking a second look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you @higgsb0. I just had one minor comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - thank you!
Thanks, @higgsb0 and @kevin-bates! |
Closes #403. This change aligns the implementation of
MappingKernelManager.start_kernel
with its docstring.It is worth mentioning that in the API Doc, there is currently no method for starting a kernel by specifying a
kernel_id
. As such, no changes are made tojupyter_server/tests/services/kernels/test_api.py
. Nevertheless, this change provides a possibility to support such functionality if that is required in future.