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

New hook point for messages / message transforms. #113

Closed
dwillmer opened this issue Mar 24, 2016 · 14 comments
Closed

New hook point for messages / message transforms. #113

dwillmer opened this issue Mar 24, 2016 · 14 comments
Milestone

Comments

@dwillmer
Copy link
Contributor

We intend to make a new hook point to allow the installation of hooks on the display publisher.

Current thoughts (largely from @minrk + @jasongrout):

  • Update ZMQDisplayPublisher to do a 2-stage message build.
  • Pass the message to the transform(s), which return a message or None.
  • If a message is returned, send it in the usual manner.

This will require:

  • Changing the display publisher to have a new method for registering a hook.
  • Changes to iostream / ipywidgets / other related repos.
@jasongrout
Copy link
Member

Also, there are issues with threading. Since the ZMQDisplayPublisher is a global object, installing a message filter there naively will affect messages from everywhere. In the case of a context manager, it seems that the "correct" behavior is to only have the output from the block of code in the current thread. @minrk suggested making the hooks thread local attributes in the ZMQDisplayPublisher object (see https://docs.python.org/2/library/threading.html#threading.local), maybe something like

from threading import local
class ZMQDisplayPublisher(...):
    def __init__(...):
        self.threadlocal = local()

    def register_hook(self, hook):
        if not hasattr(self.threadlocal, 'hooks'):
            self.threadlocal.hooks = []
        self.threadlocal.hooks.append(hook)

@jasongrout
Copy link
Member

Code to touch for making a display hook registration system includes the following.

@minrk, can you verify this is right?

@jasongrout
Copy link
Member

Interestingly, since clear_output is implemented by just sending a clear_output message to the front end, this works too:

with output_widget.capture_messages():
    clear_output()

@minrk
Copy link
Member

minrk commented Mar 24, 2016

Yup, that should be right. Also to clarify, implementing this hook should need no changes in any other repos, but the intention is to use it in a new implementation of the OutputWidget downstream.

@jasongrout
Copy link
Member

@dwillmer, @minrk: next steps:

We can get a handle on the display publisher with get_ipython().display_pub

  • Make a context manager that will register a hook and store a list of display outputs. You may need to flush something when exiting the context manager to make sure you get all of the display messages.
  • Make a widget that has such a context manager. It should take over any display_pub messages and store the messages in a list as its synced widget state. A clear_output message should clear the message queue (and probably doesn't need to be sent, since the empty message queue will be synced).
  • Make a front end widget that will render the display pub message queue (this will likely involve using a rendermime instance that is passed in...)

@minrk - can you remind us what needs to be done for capturing stdout/stderr?

@dwillmer
Copy link
Contributor Author

dwillmer commented Apr 7, 2016

message-blah3

@jasongrout - is this ^^ what you're expecting?

The print calls go to the normal output area, but any display_data messages within the new context manager are sent to the custom widget area.

Clearly we'll change the name from MessageWidget ;) just wanted some early feedback!

@jasongrout
Copy link
Member

Yep, that's the idea. Thanks!

Things to still do:

  • hook into the stdout/stderr (we also want to capture stdout). We have to write these extension points as well.
  • make sure a clear_output() message clears the messages

And when I finish the widgets in the new notebook, actually render the messages sent to the message widget.

@jasongrout
Copy link
Member

Just checking, the hook unregisters itself when the context manager exits, right? (that's not illustrated in your screenshot, but it should work that way)

@dwillmer
Copy link
Contributor Author

dwillmer commented Apr 7, 2016

yep, it does, i'll post an updated screenshot in a bit.

@minrk
Copy link
Member

minrk commented Apr 7, 2016

For stdout/err, the simple thing is to swap out sys.stdout/err to an io.StringIO object, but that won't have the same threadlocal behavior the hooks now have. To preserve that, some amount of threadlocal things will need to be added to the IOStream code, so that writes go to the short-circuit buffer rather than the default.

@jasongrout
Copy link
Member

@dwillmer - no need to post a new screenshot for the exit status. I trust it's there.

@dwillmer
Copy link
Contributor Author

widget-hook

Update:

  • Using with widget.capture(): instead of 'with widget:`.
  • capture() takes an optional positional arg for the message type, so you can intercept different messages in each context.
  • Moved context manager to ipykernel, left widget code in ipywidgets

@jasongrout
Copy link
Member

jasongrout commented Apr 23, 2016

I think this issue is solved with #115. Let's document the widget work on an issue in the ipywidgets repo. @dwillmer, can you open a work-in-progress PR over there?

@jasongrout
Copy link
Member

@minrk or @dwillmer, if this issue is solved by #115, can you close this? Apparently I don't have permissions to close issues on this repo.

@minrk minrk closed this as completed Apr 25, 2016
@minrk minrk added this to the 4.4 milestone Apr 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants