Fix print outputs in Jupyter notebook #4324
Conversation
jnumainville
requested review from
chipkent,
jmao-denver and
rcaudy
as code owners
jnumainville
added
bug
|
|
||
| def set_parent(self, parent): | ||
| # Parent needs to be set on the original stream so that output shows in proper cell | ||
| self._stream.set_parent(parent) |
There was a problem hiding this comment.
At a glance, I'm not seeing set_parent defined in https://docs.python.org/3/library/io.html#io.TextIOBase or in the io packages for cpython - what is the definition for this, should we explicitly not call super (like the other overrides we provide)?
There was a problem hiding this comment.
This is something defined by Jupyter's OutStream
https://github.com/ipython/ipykernel/blob/main/ipykernel/iostream.py#L508
The sys.stdout that is pulled at the time that the TeeStream is created is a Jupyter OutStream, so that's where the issue is.
There was a problem hiding this comment.
Sorry, I didn't read deeply enough through the linked code, see the definition in https://github.com/ipython/ipykernel/blob/6bf3fe9e44f1caf4bc371f700ac0c2e9c9c3bd84/ipykernel/iostream.py#L508-L510
I'm wondering if there are circumstances where we don't want this defined, for example to make sure we don't get something else calling set_parent on us if the stream that we wrap itself doesn't define set_parent. I'm not sure what that case would be, but some kind of intermediate "we've replaced sys.stdout too early for jupyter to have replaced it before us" situation?
I do notice that the containing type, OutStream, doesn't have any other supertypes to test if it is an instance, and the linked issue reports that jupyter is "graceful" in its handling if there is no set_parent method defined. Which isn't very graceful (since it leads to this issue) and seems like it could silently fail in other ways if we miss some other poorly documented bit of expected magic...
Backing up for a minute, the purpose of defining our own TeeStream type to begin with is so that a running server can make the full stdout available to any connecting client. @chipkent do we think that's a reasonable use case here, or if something else is mucking with stdout, should the creation of a deephaven_server.Server instead have a flag to be hands-off, and assume that jupyter will handle all the logs?
There was a problem hiding this comment.
I think that having the logs everywhere is the least confusing option for a user. I think we leave it like this until we have a compelling reason to make a change.
|
|
||
| def set_parent(self, parent): | ||
| # Parent needs to be set on the original stream so that output shows in proper cell | ||
| self._stream.set_parent(parent) |
There was a problem hiding this comment.
I think that having the logs everywhere is the least confusing option for a user. I think we leave it like this until we have a compelling reason to make a change.
|
@chipkent merged, thanks |
Fixes #4277
Essentially, the parent needs to be updated so that Jupyter's
OutputStreamcan redirect the output properlyMore info here: StanfordLegion/legion#1393
Tested with code at #4277
Initial run:
rerun: