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

Don't try to call _repr_*_ on a class #817

Merged
merged 6 commits into from
Sep 25, 2020
Merged

Conversation

tkf
Copy link
Member

@tkf tkf commented Sep 13, 2020

fix #816

An MWE

py"""
class Issue816(object):
    def _repr_html_(self):
        return "<h1>Issue816</h1>"
"""

showable("text/html", py"Issue816()")  # => true
showable("text/html", py"Issue816")  # => throws
ERROR: PyError ($(Expr(:escape, :(ccall(#= /home/takafumi/.julia/dev/PyCall/src/pyfncall.jl:43 =# @pysym(:PyObject_Call), PyPtr, (PyPtr, PyPtr, PyPtr), o, pyargsptr, kw))))) <class 'TypeError'>
TypeError("_repr_html_() missing 1 required positional argument: 'self'")

Stacktrace:
 [1] pyerr_check at /home/takafumi/.julia/dev/PyCall/src/exception.jl:62 [inlined]
 [2] pyerr_check at /home/takafumi/.julia/dev/PyCall/src/exception.jl:66 [inlined]
 [3] _handle_error(::String) at /home/takafumi/.julia/dev/PyCall/src/exception.jl:83
 [4] macro expansion at /home/takafumi/.julia/dev/PyCall/src/exception.jl:97 [inlined]
 [5] #110 at /home/takafumi/.julia/dev/PyCall/src/pyfncall.jl:43 [inlined]
 [6] disable_sigint at ./c.jl:446 [inlined]
 [7] __pycall! at /home/takafumi/.julia/dev/PyCall/src/pyfncall.jl:42 [inlined]
 [8] _pycall!(::PyObject, ::PyObject, ::Tuple{}, ::Int64, ::Ptr{Nothing}) at /home/takafumi/.julia/dev/PyCall/src/pyfncall.jl:29
 [9] _pycall! at /home/takafumi/.julia/dev/PyCall/src/pyfncall.jl:11 [inlined]
 [10] #pycall#115 at /home/takafumi/.julia/dev/PyCall/src/pyfncall.jl:80 [inlined]
 [11] pycall at /home/takafumi/.julia/dev/PyCall/src/pyfncall.jl:80 [inlined]
 [12] showable(::MIME{Symbol("text/html")}, ::PyObject) at /home/takafumi/.julia/dev/PyCall/src/PyCall.jl:901
 [13] showable(::String, ::Any) at ./multimedia.jl:77
 [14] top-level scope at /home/takafumi/.julia/dev/PyCall/src/pyeval.jl:232

The error is due to invoking Issue816._repr_html_(). A simple solution seems to just return false from showable("text/html", o) when o is a python type. Although it's probably technically possible to implement _repr_html_ on classes as well, it's very likely virtually nobody uses it.

src/PyCall.jl Outdated
@@ -897,7 +897,10 @@ for (mime, method) in ((MIME"text/html", "_repr_html_"),
throw(MethodError(show, (io, mime, o)))
end
Base.showable(::$mime, o::PyObject) =
!ispynull(o) && hasproperty(o, $method) && let meth = o.$method
!ispynull(o) &&
!pyisinstance(o, @pyglobalobj :PyType_Type) && # issue 816
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use PyCallable_Check instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue816._repr_html_ in the OP is callable so I'm not sure if PyCallable_Check is enough. But maybe PyMethod_Check would work? I'll check.

@tkf
Copy link
Member Author

tkf commented Sep 24, 2020

I considered a few approaches. I think the best way to do this to port IPython.utils.dir2.get_real_method to Julia (or we can just copy dir2.py but it doesn't seem like the traditional approach that PyCall.jl takes). I don't think it covers all edge cases I had in mind (e.g., descriptors) but we don't really need to support edge cases than IPython does. Since Python classes with special _repr_*_ methods are likely to be tested with IPython, following IPython's logic sounds like the best approach.

I think this is good to go.

(Side notes: 95ee1f1 implements one of the alternative methods by using inspect.signature. I think it's a bit more accurate than IPython's get_real_method. However, it's much more complicated. I also considered "easier to ask for forgiveness than permission" approach. But I think it's better to stick with IPython's approach.)

@stevengj
Copy link
Member

LGTM — get_real_method is a bit wacky but Python is messy at this level.

@stevengj stevengj merged commit 9380a69 into JuliaPy:master Sep 25, 2020
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

Successfully merging this pull request may close these issues.

showable error on certain objects
2 participants