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

Fix precompilation on latest master #26

Merged
merged 1 commit into from
Apr 21, 2023
Merged

Conversation

alhirzel
Copy link
Contributor

@alhirzel alhirzel commented Apr 20, 2023

Use Revise.jl's "trick" that disables __init__() when precompiling.

See: timholy/Revise.jl#731

May not be the most idiomatic or best fix in the long term; perhaps the first call to a PythonPlot function could initialize the backend instead?

May or may not be related to #25 .

Use Revise.jl's "trick" that disables __init__() when precompiling.

See: timholy/Revise.jl#731
@stevengj
Copy link
Member

LGTM.

@stevengj
Copy link
Member

Does PyPlot.jl need a similar patch?

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@7a29424). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #26   +/-   ##
=======================================
  Coverage        ?   17.33%           
=======================================
  Files           ?        5           
  Lines           ?      450           
  Branches        ?        0           
=======================================
  Hits            ?       78           
  Misses          ?      372           
  Partials        ?        0           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@stevengj stevengj merged commit 9a7d10e into JuliaPy:main Apr 21, 2023
@alhirzel alhirzel deleted the patch-1 branch April 25, 2023 20:34
@KristofferC
Copy link

So people can not use this package at top-level now when they precompile their package? What if they want to precompile some workload that plots something with PythonCall?

@BeastyBlacksmith
Copy link

So people can not use this package at top-level now when they precompile their package? What if they want to precompile some workload that plots something with PythonCall?

Indeed that is the issue I am facing with Plots.jl now

@KristofferC
Copy link

Indeed that is the issue I am facing with Plots.jl now

I guess you could manually call PythonPlot.__init__() in the workload?

@BeastyBlacksmith
Copy link

I'm getting segfaults either way, for now I have to put an upper bound on PythonPlot

@t-bltg
Copy link
Contributor

t-bltg commented Aug 30, 2023

May not be the most idiomatic or best fix in the long term; perhaps the first call to a PythonPlot function could initialize the backend instead?

Yeah well, this causes trouble for Plots. Worth reverting ?

@alhirzel
Copy link
Contributor Author

alhirzel commented Aug 30, 2023

Yeah well, this causes trouble for Plots.

Towards a better fix:

# global PyObject constants that get initialized at runtime. We
# initialize them here (rather than via "global foo = ..." in __init__)
# so that their type is known at compile-time.
const matplotlib = PythonCall.pynew()
const pyplot = PythonCall.pynew()
const Gcf = PythonCall.pynew()
const orig_draw = PythonCall.pynew()
const orig_gcf = PythonCall.pynew()
const orig_figure = PythonCall.pynew()
const orig_show = PythonCall.pynew()

I think creation of these kinds of globals must be deferred until the first call to a plotting function. @stevengj - any thoughts on why it's currently set up this way, and whether it serves us in more recent Julia versions?

Worth reverting ?

Agree, personally. Some thought is needed, but my short-term fix seems like it will not suffice.

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.

6 participants