Eagerly add kernel metadata before the kernel is initialised

[agriyakhetarpal]

This PR addresses the problem discussed with @ivonnem3 and @adamblake internally where we were receiving the HTTP 422 error code from the sharing service if we try to save/share the notebook before the Python/R kernels are initialised. I've added an ensureLanguageMetadata function that adds three fields if needed: the kernel language info (required to resolve the HTTP 422 error code), and the language info's file_extension and version to fix a further HTTP 500 error code.

I have not added a test for this change at this time; it is difficult to test it in its current state because we currently mock the API calls for the sharing service and #73 is not yet resolved. I've verified this manually by spinning up the sharing service locally and ensuring that the save and the share buttons generate a shareable URL before the kernel is loaded.

[krassowski]

and the language info's file_extension and version to fix a further HTTP 500 error code.

IMO this is a bug on the CKHub side - the specification lists these fields as optional, which is why xeus-r kernel for example does not populate them. It should be solved in CKHub. I reported it previously JupyterEverywhere/sharing-service#9 and it ought to be solved by JupyterEverywhere/sharing-service@79d8f14 - I just wonder if it was not solved across the board, or were we facing the issues due to old CKHub deployment?

[krassowski]

Also, language_info itself is not required by nbformat. I do not understand why CKHub needs to know about it.

[agriyakhetarpal]

and the language info's file_extension and version to fix a further HTTP 500 error code.

IMO this is a bug on the CKHub side - the specification lists these fields as optional, which is why xeus-r kernel for example does not populate them. It should be solved in CKHub. I reported it previously JupyterEverywhere/sharing-service#9 and it ought to be solved by JupyterEverywhere/sharing-service@79d8f14 - I just wonder if it was not solved across the board, or were we facing the issues due to old CKHub deployment?

Thanks for the links! Yes, on Friday, I somehow managed to figure out this bug independently of you, haha (see the internal Slack thread with @adamblake). The file_extension and version fields are marked as optional, but are apparently non-optional.

Also, language_info itself is not required by nbformat. I do not understand why CKHub needs to know about it.

The error message is this: InvalidNotebookException: language_info.name is required in notebook format 4.5, so I guess an empty name (because of missing metadata) fires it up, and then when you add it, it checks the rest of the fields in language_info (i.e., file_extension, version, etc.).

[agriyakhetarpal]

and the language info's file_extension and version to fix a further HTTP 500 error code.

IMO this is a bug on the CKHub side - the specification lists these fields as optional, which is why xeus-r kernel for example does not populate them. It should be solved in CKHub. I reported it previously JupyterEverywhere/sharing-service#9 and it ought to be solved by JupyterEverywhere/sharing-service@79d8f14 - I just wonder if it was not solved across the board, or were we facing the issues due to old CKHub deployment?

Thanks for the links! Yes, on Friday, I somehow managed to figure out this bug independently of you, haha (see the internal Slack thread with @adamblake). The file_extension and version fields are marked as optional, but are apparently non-optional.

Oh, I see that you've seen the thread too and responded there – never mind, thanks!

[agriyakhetarpal]

These are what look like the offending lines: https://github.com/JupyterEverywhere/sharing-service/blob/627cf993a69d4acf863fa56d16ddc037079c3fe1/src/main/java/org/jupytereverywhere/model/JupyterNotebookEntity.java#L36-L40

[krassowski]

The error message is this: InvalidNotebookException: language_info.name is required in notebook format 4.5, so I guess an empty name (because of missing metadata) fires it up, and then when you add it, it checks the rest of the fields in language_info (i.e., file_extension, version, etc.).

My point is that it is a bit arbitrary. The nbformat does not require language_info to be present at all, but if it is present then it must have name. The remaining fields are not required. All kernels and all jupyter tooling will assume that they can just follow nbformat specification because this is what specifications are for.

https://nbformat.readthedocs.io/en/latest/format_description.html#top-level-structure

If CKHub wants to be different (and if there is a good reason for it), then it should be documented somewhere outside of the codebase, otherwise issues like this will continue propping up.

[agriyakhetarpal]

Okay, that is much clearer. I think we should either keep this a draft for now and revisit it when it is handled from the backend, or go ahead with this and ask to start versioning the sharing service, as it would be good for reliability and for being able to know what changed.

[peytondmurray]

At the moment I'm not in favor of merging this, not because of any issue with what's here but because if CKHub doesn't conform to the notebook spec (i.e. it doesn't follow nbformat) it's a CKHub problem and should be solved there. In fact, making a workaround here may result in a fix being deprioritized there.

Practically speaking it if it breaks the webapp people will report it here, and if it takes too long to get things solved with CKHub we can accommodate here, but I'd really prefer not to. Let's therefore wait for the moment on this change and instead continue to encourage CKHub to get this fixed.

[peytondmurray]

Oof, typescript literal types can be cumbersome. If we end up merging this at some point, can we define KernelName and KernelLanguage literal types? e.g.

type KernelName = 'xpython' | 'xr';
type KernelLanguage = 'python' | 'r';

Then you can change KERNEL_NAME_TO_URL to use that type, and the line above becomes

const language: KernelLanguage = KERNEL_NAME_TO_URL[kernelName];

(with no need to cast).

[peytondmurray]

I'm going to move this back into draft since we are waiting on CKHub for a fix.

[agriyakhetarpal]

This PR should no longer be required, as the issue has been fixed upstream: JupyterEverywhere/sharing-service#24. I'm closing this!