These are chat archives for ipython/ipython

22nd
Oct 2014
Jason Grout
@jasongrout
Oct 22 2014 17:05
do we have a hangout time for the widget discussion?
Jonathan Frederic
@jdfreder
Oct 22 2014 17:08
I think so
but @SylvainCorlay didn't say he was available
Jason Grout
@jasongrout
Oct 22 2014 18:01
@minrk: I'm looking at the comm code in IPython/kernel/comm/comm.py, and I notice that the _closed attribute starts out as False. If a comm opens, it is then redundantly set to False. however, if there is an error opening the comm, then an error is generated in open(), but _closed remains False. This then causes a problem when you try to delete the comm, because it thinks it is open.
Do you (or @jdfreder or @takluyver) know why the comm starts out with _closed being False?
Min RK
@minrk
Oct 22 2014 18:02
It's probably just an oversight, not handling the failure to open
Jason Grout
@jasongrout
Oct 22 2014 18:03
so I'm curious if you know any ramifications of changing the default for _closed to True
Min RK
@minrk
Oct 22 2014 18:03
Starting with _closed=True, and only setting _closed=False on successful open makes sense
Jason Grout
@jasongrout
Oct 22 2014 18:03
the open() method only gets called if the python side is the primary (self.primary === True). What happens if the comm is opened from Javascript?
obviously we want _closed to be False if we open from javascript. I don't see how it will be set to False if self.open() is not run
Min RK
@minrk
Oct 22 2014 18:04
just set it directly
if primary: self.open(); else self._closed = False
Jason Grout
@jasongrout
Oct 22 2014 18:05
right, okay, that makes sense. I was running it by you to see if there was some other ramifications
there were
PR coming up
Min RK
@minrk
Oct 22 2014 18:05
If you want to do it in a method, call it something like _open_secondary. At this point, I think the only thing it would do is set _closed = False
excellent, thanks!
Jason Grout
@jasongrout
Oct 22 2014 18:05
I'll just set _closed to False for now.
Min RK
@minrk
Oct 22 2014 18:06
Is there any chance of a simple test for the failed construction? How did you run into that?
Jason Grout
@jasongrout
Oct 22 2014 18:07
sphinx was somehow running into it, and the error message said something like: Exception AttributeError: "'NoneType' object has no attribute 'send'" in <bound method Comm.__del__ of <IPython.kernel.comm.comm.Comm object at 0x2b32dafbcc50>> ignored
Min RK
@minrk
Oct 22 2014 18:07
Recovering from failed init often doesn't get handled right until init starts failing, since it's easy to make assumptions that initialization will complete.
ah, ok
Jason Grout
@jasongrout
Oct 22 2014 18:07
so my guess is that sphinx was importing it, but then it wasn't initing correctly because it wasn't running under ipython
Min RK
@minrk
Oct 22 2014 18:08
Aha, I think I might know the cause
Jason Grout
@jasongrout
Oct 22 2014 18:08
?
Min RK
@minrk
Oct 22 2014 18:08
_closed is set to True at the end of Comm.close(). If any step in close fails, _closed will not be set, so __del__ will go through the procedure again
Jason Grout
@jasongrout
Oct 22 2014 18:10
huh. So if you call close(), and something fails, then when the gc tries to collect, it will run close again. Is that what you mean?
Min RK
@minrk
Oct 22 2014 18:10
yes
so _closed can be inappropriately False if either init or close fail to complete
same bug, just in two places
I think both fixes are appropriate
Jason Grout
@jasongrout
Oct 22 2014 18:11
Sure. I'm not sure how to fix the close, though. If something fails in the close, it may need to be left open
Min RK
@minrk
Oct 22 2014 18:12
If close is called and fails, the comm should be considered dead
I would set _closed = True before doing anything else (other than checking _closed in the first place)
Jason Grout
@jasongrout
Oct 22 2014 18:15
so in open, we should probably set _closed to True at the very last
I mean set to False
right now, after _closed is set to False, we still publish a comm_open message (which might have an error)
Min RK
@minrk
Oct 22 2014 18:16
might need to be a bit careful, because publishing that message probably checks _closed
so a try/except: _closed back to False would likely be appropriate
Jason Grout
@jasongrout
Oct 22 2014 18:17
it doesn't check
_closed from True->False means I definitely succeeded in opening
Min RK
@minrk
Oct 22 2014 18:18
ok, then just setting False as the last step is fine
Jason Grout
@jasongrout
Oct 22 2014 18:18
_closed from False->True means don't send any messages---I'm either uninitialized or I've at least tried to close
Min RK
@minrk
Oct 22 2014 18:18
right
Jason Grout
@jasongrout
Oct 22 2014 18:20
Exactly how does JS instantiate a comm? Does it just call with primary=False?
ah, yes, in the manager.py, that's what happens.
where should I put tests for this? We don't have a mock comm or session object
Min RK
@minrk
Oct 22 2014 18:26
You can probably make a test for failed init by passing in a comm that just doesn't work at all.
Start a new IPython.kernel.tests.test_comm
e.g. Comm._publish_msg = lambda : raise
Jason Grout
@jasongrout
Oct 22 2014 18:28
PR at ipython/ipython#6775
I'd rather make the bigger issue of testing comms and sessions a different PR
at least to get that framework set up
Min RK
@minrk
Oct 22 2014 18:30
Sure, no problem
Just add failed init to the list of things to test