BUG: Interactive mode returned expression and not value in Jupyter #2157
Conversation
|
@datapythonista maybe you can add a tests for that here: https://github.com/ibis-project/ibis/blob/master/ibis/expr/tests/test_interactive.py let me know if it makes sense. |
|
@jreback do you mind having a look at this? It's been rebased and is green. I need this bug fixed to work on the tutorial. |
ibis/expr/tests/test_interactive.py
Outdated
| @@ -30,6 +30,12 @@ def test_interactive_execute_on_repr(self): | |||
|
|
|||
| assert len(self.con.executed_queries) > 0 | |||
|
|
|||
| def test_png_repr_returns_correct_type(self): | |||
There was a problem hiding this comment.
can you add a test when interactive is False a well
There was a problem hiding this comment.
Good point. I tried that in the past, and it's not trivial. There are different cases that affect the output of _repr_png_. Like, graphviz not being installed, anything that makes importing ibis.expr.visualize fail, or call the to_graph method. Also if the graphviz_repr option is set to False.
I added the test, to see what fails, but I think it'll be tricky to find a way to add that test in a deterministic and reliable way. We may end up introducing a test that doesn't provide much value, but it's flaky when new builds are added.
There was a problem hiding this comment.
right its just to verify that it is NOT none
There was a problem hiding this comment.
Yep, but the problem is that even with interactive set to False, _repr_png_ can still be None for different reasons:
- graphviz not installed
- option
graphviz_reprset toFalse - an exception happens when importing the visualization module
I added a test for the case when all conditions are met for the result to be not None. It's a bit tricky, but I think it's the best we can do.
ibis/expr/tests/test_interactive.py
Outdated
| assert table._repr_png_() is None | ||
|
|
||
| try: | ||
| import ibis.expr.visualize |
There was a problem hiding this comment.
this import doesn't need to be here
ibis/expr/tests/test_interactive.py
Outdated
| with config.option_context('interactive', True): | ||
| assert table._repr_png_() is None | ||
|
|
||
| try: |
ibis/expr/tests/test_interactive.py
Outdated
|
|
||
| ibis.expr.visualize.to_graph(table).pipe(format='png') | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
here just skip the test if you can't format
|
Thanks for the feedback @jreback, it was useful. This should be ready now. |
|
thanks @datapythonista |
Closes #2155
In Jupyter, interactive mode is not interactive, expressions are displayed.
I don't see
Exprmuch tested, I guess not worth of having a test only for this.