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

Added fix for display hook call output format #11101

Merged
merged 2 commits into from May 2, 2018

Conversation

Projects
None yet
3 participants
@MSeal
Contributor

MSeal commented Apr 17, 2018

Hit an issue were under complex conditions, that are hard to express in a simple notebook, the displayhook was building output array elements in the old tuple format instead of the kwarg format. This causes (ignore py2 package path in the stack trace example):

TypeErrorTraceback (most recent call last)
<ipython-input-7-dcf49c89b1af> in <module>()
----> 1 output.outputs[0]

/usr/local/lib/python2.7/dist-packages/IPython/utils/capture.pyc in outputs(self)
    112                 display(o)
    113         """
--> 114         return [ RichOutput(**kargs) for kargs in self._outputs ]

Tracing down the fix was to adjust the callable which was not updated. Tracing the capture code throughout the project was a real headache as the global state is touched a lot along the way, so I made a spec just to prove that the output format conforms rather than express where that hook callable is made consequentially.

Py2 backport PR: #11102

@MSeal

This comment has been minimized.

Contributor

MSeal commented Apr 17, 2018

Failure seems unrelated to PR? FAIL: IPython.core.tests.test_completer.test_omit__names?

@Carreau

This comment has been minimized.

Member

Carreau commented Apr 19, 2018

Failure seems unrelated to PR? FAIL: IPython.core.tests.test_completer.test_omit__names?

That might be due to a recent release of Jedi. I'll investigate.

@MSeal

This comment has been minimized.

Contributor

MSeal commented Apr 24, 2018

@Carreau Any update on this?

@takluyver

This comment has been minimized.

Member

takluyver commented Apr 27, 2018

I think this makes sense. The corresponding change to displaypub was in #10837, which shipped in 6.3. Reopening to rerun tests.

@takluyver takluyver closed this Apr 27, 2018

@takluyver takluyver reopened this Apr 27, 2018

@takluyver

This comment has been minimized.

Member

takluyver commented Apr 27, 2018

...which probably won't help, because they're failing on master as well. @Carreau , did you get anywhere with the test_omit__names failure?

@MSeal

This comment has been minimized.

Contributor

MSeal commented May 2, 2018

Looks like the tests are passing now against the master changes

@Carreau

This comment has been minimized.

Member

Carreau commented May 2, 2018

Yes we've commented the tests.

I'm flabbergasted our bot did not react to the PR merge. I guess we also need to backport that on 6.x. and do a 5.x, and 6.x release.

@Carreau Carreau added this to the 5.7 milestone May 2, 2018

@Carreau Carreau merged commit 1f9cffe into ipython:master May 2, 2018

4 checks passed

codecov/patch 100% of diff hit (target 0%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +32.81% compared to 01bd59e
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@meeseeksdev

This comment has been minimized.

Contributor

meeseeksdev bot commented May 2, 2018

There seem to be a conflict, please backport manually

meeseeksdev bot pushed a commit that referenced this pull request May 2, 2018

@Carreau

This comment has been minimized.

Member

Carreau commented May 2, 2018

Ah, no my bad, I merged the backport, and not on master that's why.

@MSeal MSeal deleted the MSeal:captureHookFixPy3 branch May 2, 2018

takluyver added a commit that referenced this pull request May 4, 2018

@Carreau Carreau modified the milestones: 5.7, 6.4 May 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment