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

Skip __init__ when precompiling #328

Closed
wants to merge 1 commit into from

Conversation

stillyslalom
Copy link

I thought __init__() was always skipped when precompiling, but this appears to not be the case (at least on Windows). This appears to fix stillyslalom/PyThermo.jl#7, and may also fix #266, JuliaPy/PythonPlot.jl#25, and JuliaPy/PythonPlot.jl#27.

@stillyslalom
Copy link
Author

@cjdoris could you approve the CI run?

@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Merging #328 (8ad6e50) into main (e374e25) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #328      +/-   ##
==========================================
+ Coverage   41.50%   41.52%   +0.01%     
==========================================
  Files          76       76              
  Lines        4652     4655       +3     
==========================================
+ Hits         1931     1933       +2     
- Misses       2721     2722       +1     
Impacted Files Coverage Δ
src/PythonCall.jl 78.94% <100.00%> (+0.56%) ⬆️
src/cpython/CPython.jl 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@ufechner7
Copy link

ufechner7 commented Jul 20, 2023

What is missing to merge this commit? The bug that this commit fixes is causing issues with my co-workers when they are trying to install PythonPlot...

@stillyslalom
Copy link
Author

Pinging @cjdoris for any comments on what's blocking merge. He may just be on vacation, as I don't see any GitHub activity for about a month.

@cjdoris
Copy link
Collaborator

cjdoris commented Jul 23, 2023

Hi yeah I've been away.

My worry with this is what happens if a package Foo that depends on PythonCall has an __init__ function which does pyimport("something")? Unless Foo.__init__ is also guarded in the same way, it assumes that PythonCall.__init__ has run already, and therefore will error because it hasn't.

Or have I misunderstood and it all works fine? I'm going to experiment a bit.

@cjdoris
Copy link
Collaborator

cjdoris commented Jul 23, 2023

Also the linked issues all look like the same issue of conda install mysteriously failing during precompilation, which I think has been reported before but not resolved. Really efforts should be concentrated on fixing that.

Edit: it's #266

@stillyslalom
Copy link
Author

I would love to figure out the underlying issue, but this band-aid PR makes this (very annoying) problem go away. Can we merge it as a minimally-intrusive stopgap?

@cjdoris
Copy link
Collaborator

cjdoris commented Jul 26, 2023

It is not minimally intrusive, it actually very easily breaks any downstream packages in the way I suspected above.

If I make a package Foo containing just

using PythonCall
function __init__()
    pyimport("sys")
end

then precompiling Foo fails because it runs Foo.__init__() which calls pyimport("sys") which only works if PythonCall.__init__() has already run.

I don't think this general idea can work, so I'm going to close this PR. Another option may be to initialise Python not in __init__ but whenever pyimport and friends are actually called - but this would be a larger change to the codebase.

@cjdoris cjdoris closed this Jul 26, 2023
@stillyslalom
Copy link
Author

But Foo shouldn't call __init__() during precompilation in the first place - the fact that it does is a bug that will probably need to be resolved within Julia itself. PythonPlot.jl already uses the suggested escape hatch to avoid the same bug: https://github.com/JuliaPy/PythonPlot.jl/blob/63ed915e9a3abcffa24552129d00b7f1b9099a56/src/init.jl#L147

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.

Precompilation after add fails for packages with python dependencies Fix precompile errors on Windows
3 participants