Make kernel_id as a conditional optional field

[allstrive]

Fix for #1299

[kevin-bates]

I think keeping kernel_id required is important, especially when the only time one won't be available is when action == "start" and status == "error".

I poked around and found the if-then-else construct can be used to introduce conditionality into schemas and, by amending the schema in this PR with the following, we can introduce the idea that kernel_id NOT be required only on failed start requests. (Note that kernel_id: type: string should be restored and the description updated to state that kernel_id is conditionally required, e.g.,: This field is required except when action is start and status is error.)

if:
  properties:
    status: 
        const: error
    action:
        const: start
else:
  required:
    - kernel_id 

I verified this behavior using https://yamlschemavalidator.datree.io/ with the following instance data and updating action and status relative to the presence/absence of kernel_id:

action: start
status: error
--kernel_id: foo
kernel_name: bar
msg: text

I don't feel this overly complicates the schema and keeps kernel_id required for the majority (88%) of the action/status combinations. That said, I'd like @Zsailer's input and have requested his review.

Here's the suggested schema:

"$id": https://events.jupyter.org/jupyter_server/kernel_actions/v1
version: 1
title: Kernel Manager activities
personal-data: true
description: |
  Record events of a kernel manager.
type: object    
required:
  - action
  - msg
properties:
  action:
    enum:
      - start
      - interrupt
      - shutdown
      - restart
    description: |
      Action performed by the Kernel Manager.

      This is a required field.

      Possible values:

      1. start
         A kernel has been started with the given kernel id.

      2. interrupt
         A kernel has been interrupted for the given kernel id.

      3. shutdown
         A kernel has been shut down for the given kernel id.

      4. restart
         A kernel has been restarted for the given kernel id.
  kernel_id:
    type: string
    description: |
      Kernel id.
      
      This field is required except when action is start and status is error.
  kernel_name:
    type: string
    description: |
      Name of the kernel.
  status:
    enum:
      - error
      - success
    description: |
      Status received from a rest api operation to kernel server.

      This is a required field.

      Possible values:

      1. error
         Error response from a rest api operation to kernel server.

      2. success
         Success response from a rest api operation to kernel server.
  status_code:
    type: number
    description: |
      Http response codes from a rest api operation to kernel server.
      Examples: 200, 400, 502, 503, 599 etc
  msg:
    type: string
    description: |
      Description of the event specified in action.
if:
  properties:
    status: 
        const: error
    action:
        const: start
else:
  required:
    - kernel_id 

[allstrive]

Thanks @kevin-bates for that wonderful suggestion 😄
Learnt something today - conditional validation in schema. I will give it a try and post an update here.
Have a great weekend.

[kevin-bates]

This stanza might be a bit more straightforward (and direct):

# Do not require kernel_id on failed start attempts
if:
  not:
    properties:
      status: 
        const: error
      action:
        const: start
then:
  required:
    - kernel_id 

[rajmusuku]

lgtm

[allstrive]

This change is required to ensure that , it doesn't produce message with value
kernel_id : null as that will fail with current schema. The current schema won't accept null value.

[kevin-bates]

Thanks for the updates. This looks good to me.

I suspect the build failures are unrelated, albeit related to JSON schema. However, since the first commit on the PR also failed with a similar issue, I don't think the failure is related to the use of the conditional.

I'm hoping we can get @Zsailer's take on this before merging.

[blink1073]

I just pushed a fix to ignore that deprecation warning for now, as I did in pytest-jupyter. The true fix will involve some refactoring in jupyter_events to handle the new resolver API in jsonschema.

[Zsailer]

Apologies for the delayed response here.

I poked around and found the if-then-else construct can be used to introduce conditionality into schemas

Nice find @kevin-bates! Thank you for taking the time to dive into JSON schemas and find this.

This PR looks good to me. Just for future reference—Since this is a, in essence, a bug fix in the schema, there's no need to version the schema.

[allstrive]

@blink1073 We are still seeing jsonschema related error , this time it is downstream in jupyterlab-server. Do we need it to be all green for the PR to be merged ? If so, then we will have to apply the similar fix to jupyterlab-server that you did for jupyter-server.

[blink1073]

That looks like a problem for jupyterlab_server to solve, not related to this PR, merging!