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

JEP for Optional Features #92

Merged
merged 5 commits into from
Jun 26, 2023
Merged

Conversation

JohanMabille
Copy link
Member

@JohanMabille JohanMabille commented Jan 16, 2023

Following this discussion, here is the JEP for introducing optional features in the protocol.

Voting from @jupyter/software-steering-council

@kevin-bates
Copy link
Member

Hi @JohanMabille - this is a good idea - thanks for writing it up.

I never know how detailed a JEP should be but have a couple of questions that may (or may not) be part of this JEP.

  1. Where will the canonical list of feature names be maintained? (I'm figuring in the messaging documentation in jupyter_client but this should be clarified). Seems like this area should also describe the feature and its expected behavior so kernel/feature authors know what is required.
  2. Should there be a corresponding version indicator associated with each feature (and whose syntax is specified in this proposal, e.g., semantic versioning, strictly increasing integer, etc.)?

@JohanMabille
Copy link
Member Author

Where will the canonical list of feature names be maintained? (I'm figuring in the messaging documentation in jupyter_client but this should be clarified). Seems like this area should also describe the feature and its expected behavior so kernel/feature authors know what is required.

Indeed, like the other parts of the protocol, this list should be in the messaging documentation. I don't know if this has to be specified in the JEP though.

Should there be a corresponding version indicator associated with each feature (and whose syntax is specified in this proposal, e.g., semantic versioning, strictly increasing integer, etc.)?

I'm afraid that doing so can lead to unnecessary complexity when the number of optional features grows. I think it's ok to bump the major version of the protocol even if only optional features are backward incompatible.

@minrk
Copy link
Member

minrk commented May 4, 2023

Does it make sense for this to live (also?) at the kernelspec level? I'm thinking of #66, where handshaking may be considered a feature, rather than an unconditional version>= requirement. 👍 in general.

@minrk minrk mentioned this pull request May 22, 2023
33 tasks
@JohanMabille
Copy link
Member Author

JohanMabille commented Jun 5, 2023

The kernel handshake pattern itself should not be optional as discussed in #66. For other features, I think that as long as they do not concern the connection itself or data that a client should get before connecting to the kernel, having them in the kernel_info message is enough, no need to duplicate these information in the kernelspec.

I cannot think of an optional feature that would required to be in the kernelspec; if we have one in the future, maybe we can add a new field to the kernelspec in a dedicated PR?

@minrk
Copy link
Member

minrk commented Jun 6, 2023

That sounds reasonable to me, thanks!

@JohanMabille
Copy link
Member Author

Here is a potential diff of the Kernel Protocol documentation when we add the Optional Features:

--- a/docs/messaging.rst
+++ b/docs/messaging.rst
@@ -246,6 +246,24 @@ block waiting for a reply, so the frontend must answer.
 Both sides should allow unexpected message types, and extra fields in known
 message types, so that additions to the protocol do not break existing code.
 
+.. _optional_features:
+
+Optional Features
+=================
+
+Optional features are parts of the protocol that kernels are not required to
+implement. An optional feature can be a list of additional messages and/or a
+list of additional fields in different existing messages. When a feature introduces
+new messages, it is its responsibility to specify the order of these messages when
+it makes sense. Under no circumstances this feature should alter the order of already
+existing messages, nor interleave new messages between already existing messages.
+
+Kernels can tell the frontend which optional features they support thanks to the
+``'supported_features'`` field in the :ref:`kernel_info_reply <msging_kernel_info>`.
+
+Available optional features are:
+- debugger: :ref:`debug request <debug_request>`, :ref:`debug event <debug_event>`
+
 .. _wire_protocol:
 
 The Wire Protocol
@@ -998,8 +1016,13 @@ Message type: ``kernel_info_reply``::
 
         # A boolean flag which tells if the kernel supports debugging in the notebook.
         # Default is False
+        # 'debugger' is considered as deprecated, prefer the 'supported_features' list
+        # instead
         'debugger': bool,
 
+        # Optional: The list of optional features supported by the kernel
+        'supported_features': [str],
+
         # Optional: A list of dictionaries, each with keys 'text' and 'url'.
         # These will be displayed in the help menu in the notebook UI.
         'help_links': [
@@ -1010,6 +1033,9 @@ Message type: ``kernel_info_reply``::
 Refer to the lists of available `Pygments lexers <http://pygments.org/docs/lexers/>`_
 and `codemirror modes <http://codemirror.net/mode/index.html>`_ for those fields.
 
+Refer to the :ref:`Optional Features section <_optional_features>` for the list of
+available optional features.
+
 .. versionchanged:: 5.0
 
     Versions changed from lists of integers to strings.
@@ -1106,8 +1132,16 @@ Message type: ``interrupt_reply``::
 
 .. versionadded:: 5.3
 
-Debug request
--------------
+.. _debug_request:
+
+Debug request (Optional Feature: debugger)
+------------------------------------------
+
+.. note::
+
+    This message is part of the debugger optional feature. Kernels should indicate
+    to the front end that they support it by appending the ``debugger`` string
+    to the ``supported_features`` field of the ``kernel_info_reply`` message.
 
 This message type is used with debugging kernels to request specific actions
 to be performed by the debugger such as adding a breakpoint or stepping into
@@ -1574,8 +1608,14 @@ Message type: ``clear_output``::
 
 .. _debug_event:
 
-Debug event
------------
+Debug event (Optional feature: debugger)
+----------------------------------------
+
+.. note::
+
+    This message is part of the debugger optional feature. Kernels should indicate
+    to the front end that they support it by appending the ``debugger`` string
+    to the ``supported_features`` field of the ``kernel_info_reply`` message.
 
 This message type is used by debugging kernels to send debugging events to the
 frontend.

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.

The kernel handshake pattern itself should not be optional as discussed in #66.

This point is still under discussion AFAK.

Apart from that comment, I am OK with the proposed changed listed in the JEP.

@JohanMabille
Copy link
Member Author

JohanMabille commented Jun 13, 2023

The kernel handshake pattern itself should not be optional as discussed in #66.

This point is still under discussion AFAK.

It's not, as stated in the JEP:

"This pattern is NOT a replacement for the current connection pattern. It is an additional one and kernels will have to implement both of them to be conformant to the Jupyter Kernel Protocol specification. Which pattern should be used for the connection is decided by the kernel launcher, depending on the information passed in the initial connection file."

The point that was discussed was wether that new pattern should replace the old one, and the conclusion was they should live together (there are use cases for which we cannot drop the old connection pattern).

@fcollonval
Copy link
Contributor

fcollonval commented Jun 26, 2023

The vote is now closed with the results:

In favor: 7 (Eric, Frederic, Itay, Johan, Min, Sylvain, Zach)
Against: 0
Abstention: 0
No vote: 4 (Carol, Isabela, Paul, Rick)

--> In light of those results, this JEP is accepted.

@fcollonval fcollonval merged commit d4d0ef7 into jupyter:master Jun 26, 2023
@JohanMabille JohanMabille deleted the optional_features branch July 5, 2023 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants