General Fixes for Comms

[dwillmer]

Howdy,

This is a general fix-up across the Comm and CommManager classes - there was quite a lot of redundant/duplicate code which has been cleaned up. There’s also a few performance items here - eg. multiple identical dict lookups in the same function.

I deliberately haven’t made whitespace changes, but the editor has auto-cleaned up some trailing whitespace.

The biggest changes (which need input from @minrk @takluyver) are the removal of session and iopub_socket from Comm and CommManager. The tests pass and i can't find any places where these items are used directly, but it could do with some thought from you.

Their use on CommManager was only to be passed into the Comm objects (which happened in 2 places), and their use on Comm objects is restricted to a single method, so it's simpler to just take them from the kernel object that needs passing down the hierarchy anyway.

ipykernel/comm/manager.py

• Removed lazy_keys function as it’s completely broken. There was only one use which was in a logging statement, and there was an error in the function definition because the single dikt argument wasn’t used. This would have caused a runtime exception had the code ever been hit. Have replaced the single use with a direct check of the logging level, which is more obviously correct to readers.
• Removed incorrect comment - # call, because we store weak refs. The item wasn’t called, and it’s not storing weak refs.
• Fixed get_comm to not perform the dict lookup twice.
• Removed session from CommManager. It’s not used internally, and the only instantiation of a comm manager i can find is on the kernel, in which case kernel.session is the correct, direct lookup.
• Reduced duplication of code between comm_msg and comm_close.
• Removed duplication of code between register_comm and comm_open. comm_open calls register_comm anyway, so theres no need to set the shell, kernel and iopub_socket twice.
• Removed iopub_socket, because with the previous change it’s only used internally in a single place, and there’s no need for the CommManager to know about it (can be taken directly from kernel.
• Removed shell, because with the previous changes it’s only used when being set on the comm objects, and they can just take it from the kernel object.
• Removed unused imports.
• Removed an EOL semi-colon.

Overall these changes reduce the line count by about 20%, whilst also making the code more efficient, and easier to understand (i.e., smaller methods).

ipykernel/comm/comm.py

• Remove shell trait, because it’s only used in one method (and only then if a message callback is set), and there’s no need for the comm objects to know about it when that method can pull it off of kernel.
• Remove iopub for the same reason as shell above.
• Remove session for the same reason.

ipykernel/ipykernel.py

• Removed the redundant shell and parent args from being passed into CommManager.

[minrk]

This should still pass parent=self

[dwillmer]

Ok, what's the reason for needing parent set?

If it's required, we should just remove the kernel arg and use parent internally then.

[minrk]

parent passes config. Both parent and kernel should be passed, because it is not required for parent to be the kernel, only a Configurable.

[minrk]

Thanks, @dwillmer!

[dwillmer]

Thanks @minrk

[SylvainCorlay]

This breaks widgets. (Comm messages from the front end don't get handled). Should we revert it?

@minrk @jasongrout