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

WIP: Warn top-level pyimport during pre-compilation #634

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tkf
Copy link
Member

@tkf tkf commented Jan 24, 2019

This PR adds a warning from pyimport when a user tried to import Python modules at precompile time at top-level scope. Example output:

┌ Error: Python module `sys` imported at top-level scope in /home/takafumi/.julia/dev/PyPlot/src/plot3d.jl:4.
│
│ This is very likely to be a miss-use of PyCall API.  Importing Python
│ modules _MUST_ be done in `__init__` function.  Please read:
│ https://github.com/JuliaPy/PyCall.jl#using-pycall-from-julia-modules
│
│ If importing Python code at import time cannot be avoided, use
│ `disable_warn_pyimport_at_toplevel` to disable this warning within a
│ restricted code block.
└ @ PyCall ~/.julia/dev/PyCall/src/PyCall.jl:506

This solution probably sounds like too "pythonic" because it needs to look at stacktrace() and dynamically determine the scope. But jl_generating_output is the only overhead at run-time so I guess it's fine? Other concern is that I'm probably using a lot of non-public interface. But if it makes sense to do this in PyCall maybe we can request the public APIs to implement this PR?

location = ""
end
@error """
Python module `$name` imported at top-level scope$location.
Copy link
Member Author

Choose a reason for hiding this comment

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

I just realized I can do what Base.depwarn does here and pass _file and _line to @logmsg. I'll do it if this entire patch makes sense.

Run function `f` while disabling warning about top-level Python
imports.
"""
function disable_warn_pyimport_at_toplevel(f)
Copy link
Member Author

Choose a reason for hiding this comment

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

It makes sense to warn from py"..." etc. So this function should be renamed. Maybe disable_toplevel_warning.

@stevengj
Copy link
Member

stevengj commented Jan 24, 2019

While this is attractive in principle, it's perfectly legitimate to call pyimport in top-level code as long as the usage is transient. Conversely, you would have the same problem for const foo = PyObject(0) in top-level code, or any code that tries to store a PyObject in the precompiled image — not just pyimport.

The right thing would be if we can issue a warning or error when any PyObject is serialized into the precompiled image. I'm not sure if this is feasible?

(Ideally, we could just define Base.precompile_isunsafe(o::PyObject) = ispynull(o) and it would issue a warning if precompilation attempts to serialize that object. This would be a nice feature to add to Base.)

@stevengj
Copy link
Member

stevengj commented Jan 24, 2019

Hmm, I guess Base.precompile_isunsafe(o::PyObject) = ispynull(o) wouldn't actually work, because precompilation runs __init__ and actually does serialize the PyPtr — it's just that well-written code would ignore the serialized value by overwriting it in __init__.

Alternatively, if we could somehow get precompilation to use our pickle serialization, then we could maybe allow precompilation with Python objects. The precompilation code uses very low-level serialization (in julia/src/dump.c), however, and I don't immediately see any way to hook into it.

@tkf
Copy link
Member Author

tkf commented Jan 24, 2019

it's perfectly legitimate to call pyimport in top-level code as long as the usage is transient.

Yeah, so that's why I included disable_warn_pyimport_at_toplevel which probably should be called disable_toplevel_warning. It can be used like

const pypi = PyCall.disable_toplevel_warning() do
    pyimport("math").pi
end

Conversely, you would have the same problem for const foo = PyObject(0) in top-level code, or any code that tries to store a PyObject in the precompiled image — not just pyimport.

I was thinking to do this also with py"..." as well. But there are probably too many entry points to cover. At the same time, I think pyimport and py"..." cover most of the newbie traps.

if we could somehow get precompilation to use our pickle serialization

Wow. It would be great if we can support this.

@stevengj
Copy link
Member

@vtjnash, is there some way to hook into how precompilation serializes an object? I looked at src/dump.c and it seems quite difficult to alter from Julia code, but maybe I'm missing something.

@tkf
Copy link
Member Author

tkf commented Jan 24, 2019

if we could somehow get precompilation to use our pickle serialization

Wow. It would be great if we can support this.

Actually, let me take it back. A Python package may be updated after a Julia package using it is precompiled. It then could easily break pickled object when some relevant internal of the updated Python package is changed (e.g., class property is renamed). So, I think it's much safer to disallow it.

@tkf
Copy link
Member Author

tkf commented Jan 24, 2019

A Python package may be updated after a Julia package using it is precompiled.

One solution would be to run include_dependency on all Python module files (say) existed in sys.modules at precompilation time. But it perhaps is too crazy...

@vtjnash
Copy link
Contributor

vtjnash commented Jan 24, 2019

@stevengj no, there are generally not any hooks in there

@tkf tkf changed the title RFC: Warn top-level pyimport during pre-compilation WIP: Warn top-level pyimport during pre-compilation Mar 28, 2019
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.

None yet

3 participants