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 compilation failure on Julia 1.10 #6

Merged
merged 3 commits into from
Aug 10, 2023

Conversation

MilesCranmer
Copy link
Contributor

@MilesCranmer MilesCranmer commented Aug 8, 2023

Julia 1.10 requires that no I/O resources are left open during module initialization. Thus, the code:

__init__() = start()

needs to be removed, and the user must call

PlotlyKaleido.start()

manually upon initialization.

I have implemented these changes. I have also updated the package version to 2.0 so that this does not introduce breaking changes in any existing dependent projects (though they will soon break anyways).

I also fixed a bug where kill() is defined incorrectly, as it conflicts with Base.kill(). It is now kill_kaleido.

Fixes #5

@BeastyBlacksmith would you be willing to merge this relatively quickly? This is currently breaking my package AirspeedVelocity.jl on Julia 1.10.

Copy link
Member

@BeastyBlacksmith BeastyBlacksmith left a comment

Choose a reason for hiding this comment

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

I am okay with that. @joshday, what is your opinion?

@joshday
Copy link
Collaborator

joshday commented Aug 10, 2023

I don't like that you have to separately call PlotlyKaleido.start() but if it's unavoidable then it's unavoidable. LGTM

@MilesCranmer
Copy link
Contributor Author

Thanks, please merge when ready. Indeed it is a hard requirement of Julia 1.10, otherwise precompilation of downstream packages will hang [source]:

If you open an I/O resource during module definition, you now (Julia 1.10+) have to close it before exiting the module. Example of a fix:

I can't remember if the 2nd PR was needed to fix precompilation, but I don't think so. But digging turned up more evidence that we were doing a bad job of freeing resources, so responding to the changes in 1.10 made the package better.

@BeastyBlacksmith BeastyBlacksmith merged commit d0c390c into JuliaPlots:main Aug 10, 2023
4 checks passed
@MilesCranmer MilesCranmer deleted the fix-1.10 branch August 10, 2023 14:31
@KristofferC
Copy link

An alternative is perhaps to only run it when precompilation is not happening?

@timholy
Copy link

timholy commented Aug 10, 2023

What's tricky is that it doesn't affect precompilation of PlotlyKaleido, because during precompilation we don't call __init__. (Maybe we should, though.) Instead, __init__ only gets called by packages that use PlotlyKaleido. And they'd have to be responsible for killing it.

But thinking about it again, I guess you're proposing that the __init__ checks if we're precompiling, and indeed that would fix the problem for all precompilation.

@BeastyBlacksmith
Copy link
Member

guess you're proposing that the __init__ checks if we're precompiling,

How could we do that?

@timholy
Copy link

timholy commented Aug 11, 2023

Magic incantation: ccall(:jl_generating_output, Cint, ()) == 1

@MilesCranmer
Copy link
Contributor Author

PyPlot.jl uses this trick but somehow it still runs into the I/O issue seen here: https://github.com/JuliaPy/PyPlot.jl/blob/343430ac4f822a84a5e19bf5dabf7a846b171c9b/src/init.jl#L171-L209

JuliaPy/PyPlot.jl#572

@KristofferC
Copy link

PyPlot.jl uses this trick but somehow it still runs into the I/O issue seen here:

I mean, either the code that starts the process runs or not. So are you saying it still runs even with that guard?

@MilesCranmer
Copy link
Contributor Author

I haven’t debugged the details so I can’t answer. But PyPlot.jl does have the I/O issue and I couldn’t find any other I/O resources other than what’s in that __init__ and protected by that guard.

@timholy
Copy link

timholy commented Aug 11, 2023

I'll tackle this (but not today), I want to write up a little guide on how you debug these things as I go.

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.

Incompatibility with Julia 1.10 due to zombie I/O resources
5 participants