Check language_info key in kernel info received message content #96
Conversation
| if 'language_info' in info_msg['content']: | ||
| self.nb.metadata['language_info'] = info_msg['content']['language_info'] | ||
| else: | ||
| raise RuntimeError( |
There was a problem hiding this comment.
Out of interest, should this raise an error or a warning, i.e. is language_info a strictly required component of the final notebook and/or does the kernel "guarantee" to reply with it given a successful execution?
e.g. in the specification https://nbformat.readthedocs.io/en/latest/format_description.html#notebook-metadata it is not present
There was a problem hiding this comment.
I have limited knowledge of what this is actually doing lol,
but from:
https://github.com/chrisjsewell/jupyter_client/blob/778fa85c8ce7cd1a8ffe2fec2c1b8522d6c314ba/jupyter_client/adapter.py#L140
and
https://github.com/chrisjsewell/jupyter_client/blob/778fa85c8ce7cd1a8ffe2fec2c1b8522d6c314ba/jupyter_client/adapter.py#L251
it looks like if you are using the version 4 msg spec, it is not guaranteed, but if you use the version 5 msg spec it is (because of the setdefault)
There was a problem hiding this comment.
It seems to be optional in the notebook, but mandatory in the Jupyter protocol from version 5.0:
Changed in version 5.0: language_info, implementation, implementation_version, banner and help_links keys are added.
So I guess we should handle it differently depending on the version? Good catch by the way!
There was a problem hiding this comment.
no problem 😄
So I guess we should handle it differently depending on the version?
I guess so, although note the protocol did change many years ago I think, so this should be an edge case?
just baffled by why there is even an error on RTD, the only difference between these runs is that in the second I tried using your fork, which passes 🤷 (jupyter-client is the same version in both)
There was a problem hiding this comment.
Out of curiosity, can you try with nbclient master?
There was a problem hiding this comment.
Yeh I tried and its the same for master and 0.4.1 tag.
I think I've spotted the difference though: readthedocs/readthedocs.org#7360
There was a problem hiding this comment.
I don't see any reason to be backwards compatible to 4.x msg spec, so I think this is safe to make strict. 5 has been the defacto for a long time and I don't think there's many tools that work with 4 anymore.
|
Feel free to merge whenever -- we could add a test for this but I don't see it adding much benefit to capturing expected behavior in this error state. |
Fixes #95