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

Add a signal for any kernel message sent/received #4437

Merged
merged 5 commits into from May 28, 2018

Conversation

@vidartf
Copy link
Member

@vidartf vidartf commented Apr 23, 2018

A low-level hook for those interested in monitoring messages to/from the kernel.

Needed for #4423.

A low-level hook for those interested in monitoring messages to/from the kernel.
@vidartf
Copy link
Member Author

@vidartf vidartf commented Apr 23, 2018

PS: I'm open for any better names for the signal!

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented May 15, 2018

Thanks for this one @vidartf - @ian-r-rose will start to review it.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 16, 2018

Hi @vidartf, thanks for this PR, and sorry for the slow turnaround. This mostly looks good to me, I have two questions:

  1. The Session class forwards signals from its kernel, including the iopubMessage and unhandledMessage. Do you think it would be a good idea to include your anyMessage there as well?
  2. It would be nice to add some tests for this. I think they could be mostly copy-pasted from the existing message tests.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 16, 2018

This also may (or may not?) impact #4188.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 16, 2018

Would it be helpful to include in the signal some metadata about the message, in particular if it is going to a kernel or coming from a kernel, and what channel? Or should we force the listener to inspect every message to get that information?

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 16, 2018

It seems to me like it would be redundant to add metadata to the signal that can be found by inspecting the message metadata.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 16, 2018

Do you expect #4188 to be fixed for beta 3?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 16, 2018

I'm hoping to work on #4188 for beta 3. I saw that and the context refactor at #4453 as the major API stabilization things I would work on for beta 3.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 16, 2018

The message.channel attribute is the channel - fair enough on that one. However, it's a bit trickier to see if the message is going to the kernel or coming from the kernel. Basically, you have to have a lookup table of message types + channels to directions (message type isn't enough, because comm messages can be on either the shell or the iopub channel, and channel isn't enough because messages go both directions on the shell channel).

@vidartf
Copy link
Member Author

@vidartf vidartf commented May 16, 2018

Adding a direction flag sounds reasonable, but I wouldn't add anything more than that. I'm leaning towards having the signals fire synchronously, but I haven't worked it through, so I'll leave that decision to someone else. That being said, for my use case it is probably useful to log the message before anything possibly goes wrong in the processing (debugging).

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 16, 2018

I'm fine with just a direction flag, and I'm fine with a synchronous signal for now. We should clearly document that you should not modify the message (hmmm unless you want to register something that transforms messages...)

@vidartf vidartf closed this May 23, 2018
@vidartf vidartf reopened this May 23, 2018
@vidartf
Copy link
Member Author

@vidartf vidartf commented May 23, 2018

The appveyor failure seemed unrelated, so I toggled the PR for another go.

@vidartf
Copy link
Member Author

@vidartf vidartf commented May 24, 2018

@ian-r-rose I think I've addressed your initial points now as well.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

It seems like, since default.ts has the default implementation of the IKernel interface, the IAnyMessage should be part of the interface, rather than the DefaultKernel namespace. I think it would be cleaner to have it be KernelMessage.IAnyMessage

* Arguments interface for the anyMessage signal.
*/
export
interface IAnyMessageArgs {
Copy link
Member

@ian-r-rose ian-r-rose May 24, 2018

Why put this interface in default.ts rather than kernel.ts with the other message types?

Copy link
Member Author

@vidartf vidartf May 25, 2018

Thanks, I got confused about what depended on what.

Copy link
Member

@ian-r-rose ian-r-rose May 25, 2018

Sorry, I meant putting the AnyMessage interface in the KernelMessage namespace with the rest of the message definitions, not the Kernel namespace.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

LGTM, thanks!

@ian-r-rose ian-r-rose merged commit 77aa4f7 into jupyterlab:master May 28, 2018
2 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 28, 2018

I think it makes sense to have IAnyMessage closely tied to the signal (i.e., where it is now), rather than trying to pretend that it's some sort of official message.

}).catch(done);
});

it('should not be emitted for a different client session', () => {
Copy link
Contributor

@jasongrout jasongrout Jun 21, 2018

@vidartf - what is the purpose for this test? I think the test should be removed, and that anyMessage should emit on any message a kernel receives. That's what the code does.

(Note that I think when this PR was merged, these tests were not actually being run, so this test didn't show a failure.)

Copy link
Contributor

@jasongrout jasongrout Jun 21, 2018

(I'm removing this test in #4697 - I just wanted to make sure it made sense to remove it.)

Copy link
Member Author

@vidartf vidartf Jun 21, 2018

I agree this can be removed. It was a modified version of the similar one for unhandledMessage above, but I dont think that translates here.

@jasongrout jasongrout added this to the 0.33 milestone Jul 19, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants