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

Windows error for PlotlyKaleido breaks precompilation #482

Closed
disberd opened this issue Mar 4, 2024 · 8 comments · Fixed by JuliaPlots/PlotlyKaleido.jl#18
Closed

Windows error for PlotlyKaleido breaks precompilation #482

disberd opened this issue Mar 4, 2024 · 8 comments · Fixed by JuliaPlots/PlotlyKaleido.jl#18

Comments

@disberd
Copy link
Member

disberd commented Mar 4, 2024

This recent PlotlyKaleido PR JuliaPlots/PlotlyKaleido.jl#17 introduced an error when the kaleido process hangs and does not accept inputs (this should only happen on Windows 10/11 when using Kaleido_jll v0.2).

This was done as on many windows machine (but apparently not all), the execution of the Kaleido_jll library (only for version > 0.1) hangs soon after start.

This is reflected in the diverse amount of related issues in the various kaleido repositories (e.g. #473, JuliaPlots/PlotlyKaleido.jl#13, plotly/Kaleido#134, plotly/Kaleido#110 and others) and is a problem of the kaleido C library that is used by all the various plotly packages to export figures.

This problem is only relevant to some windows machines, and the wide-spread suggested solution is to download to Kaleido (C library) 0.1 to avoid the problem.

The PR in JuliaPlots/PlotlyKaleido.jl#17 caused the call to PlotlyKaleido.start() to actually throw an error if the underlying process does not appear to respond (which again should only happen on Windows and for Kaleido v0.2), as the kaleido library is anyhow useless in those situation and at least an error with a suggestion on a possible fix is (in my opinion, the author of the PR) is better than a forever hanging process.

Now, in a comment to the aforementioned PR, @montyvesselinov made me notice that this breaks precompilation of PlotlyJS even if he doesn't actually use Kaleido.

I see that a call to Kaleido.start() is happening in the __init__() function of PlotlyJS:

PlotlyJS.jl/src/PlotlyJS.jl

Lines 102 to 105 in 75eacf4

if ccall(:jl_generating_output, Cint, ()) != 1
# ensure precompilation of packages depending on PlotlyJS finishes
PlotlyKaleido.start()
end

This was added in #481 but was actually there from the start.

My question is now the following:
Do we need to pre-start the kaleido library in the init of PlotlyJS to achieve maximum TTFX when calling savefig?

I see three possible ways to solve the precompilation error:

  1. Avoid calling PlotlyKaleido.start in the __init__ function, so that it's only actually started when someone actually calls savefig (this should already happen as the savefig code is already checking for the running process)
  2. Change to code in PlotlyKaleido.jl so that the error is not thrown during the PlotlyKaleido.start() call, but when the savefig function is called (similar to 1., but might speed up TTFX of savefig for PlotlyJS only in case the user is not on Windows with 0.2.1 of Kaleido).
  3. Change the code in PlotlyKaleido to avoid throwing an error (or avoid doing it by default), but this does not actually changes the fact that if the process hangs, Kaleido is not usable and so a different library version must be installed (i.e. 0.1) for it to be used

I expressed the possible solution that came to my mind in my order of preference.
If 2 was to be implemented, for consistency I believe we should avoid throwing errors all-together in PlotlyKaleido.start as also the other currently present errors are thrown if the library is not actually usable.

@BeastyBlacksmith, do you have any comment on this and suggestions on proposed way-forward?

@montyvesselinov
Copy link

This needs to be fixed. I do not think it is ok PlotlyKaleido to break the compilation of PlotlyJS. In many cases, PlotlyKaleido is not used at all when PlotlyJS is executed.

This should be just a warning message.

@BeastyBlacksmith
Copy link
Member

I do agree that PlotlyJS should remain usable even if the kaleido process hangs. I also think it should issue a warning at using time if that is the case to prevent surprises when trying to save.

@disberd
Copy link
Member Author

disberd commented Mar 4, 2024

This is the error message that is just logged with JuliaPlots/PlotlyKaleido.jl#18

Precompilation doesn't fail but savefig will not work. It will actually error (after re-displaying the log error as it will try to restart the kaleido process)

image

@montyvesselinov
Copy link

For me it fails:

               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.10.2 (2024-03-01)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> import PlotlyJS
ERROR: InitError: It looks like the kaleido process is hanging.
If you are on windows this might be caused by known problems with Kaleido v0.2 on windows.
You might want to try forcing a downgrade of the kaleido library to 0.1.
Check the Package Readme at https://github.com/JuliaPlots/PlotlyKaleido.jl/tree/main#windows-note for more details
Stacktrace:
  [1] error(s::String)
    @ Base .\error.jl:35
  [2] readline_noblock
    @ C:\Users\monty\.julia\packages\PlotlyKaleido\aujjS\src\PlotlyKaleido.jl:52
  [3] #start#6
    @ C:\Users\monty\.julia\packages\PlotlyKaleido\aujjS\src\PlotlyKaleido.jl:128
  [4] start
    @ C:\Users\monty\.julia\packages\PlotlyKaleido\aujjS\src\PlotlyKaleido.jl:59 [inlined]
  [5] __init__()
    @ PlotlyJS C:\Users\monty\.julia\packages\PlotlyJS\b6MbQ\src\PlotlyJS.jl:104
  [6] run_module_init(mod::Module, i::Int64)
    @ Base .\loading.jl:1134
  [7] register_restored_modules(sv::Core.SimpleVector, pkg::Base.PkgId, path::String)
    @ Base .\loading.jl:1122
  [8] _include_from_serialized(pkg::Base.PkgId, path::String, ocachepath::String, depmods::Vector{Any})
    @ Base .\loading.jl:1067
  [9] _require_search_from_serialized(pkg::Base.PkgId, sourcepath::String, build_id::UInt128)
    @ Base .\loading.jl:1581
 [10] _require(pkg::Base.PkgId, env::String)
    @ Base .\loading.jl:1938
 [11] __require_prelocked(uuidkey::Base.PkgId, env::String)
    @ Base .\loading.jl:1812
 [12] #invoke_in_world#3
    @ .\essentials.jl:926 [inlined]
 [13] invoke_in_world
    @ .\essentials.jl:923 [inlined]
 [14] _require_prelocked(uuidkey::Base.PkgId, env::String)
    @ Base .\loading.jl:1803
 [15] macro expansion
    @ .\loading.jl:1790 [inlined]
 [16] macro expansion
    @ .\lock.jl:267 [inlined]
 [17] __require(into::Module, mod::Symbol)
    @ Base .\loading.jl:1753
 [18] #invoke_in_world#3
    @ .\essentials.jl:926 [inlined]
 [19] invoke_in_world
    @ .\essentials.jl:923 [inlined]
 [20] require(into::Module, mod::Symbol)
    @ Base .\loading.jl:1746
during initialization of module PlotlyJS

julia> @isdefined PlotlyJS
false

@disberd
Copy link
Member Author

disberd commented Mar 4, 2024

I was just showing an example putting in an environment PlotlyJS and the dev branch of the PR.

The fix on PlotlyKaleido is not merged yet. I guess you were just trying with latest PlotlyJS with the regular published version of PlotlyKaleido?

@montyvesselinov
Copy link

Thank you! I will wait! However, I still think it is better to label this as WARNING rather than ERROR. But I might be wrong.

@disberd
Copy link
Member Author

disberd commented Mar 5, 2024

@montyvesselinov try again and make sure that while adding PlotlyJS version 2.2.4 of PlotlyKaleido (the latest) is installed.

Now you should only get a warning during using or precompilation and get an error only when trying to call savefig

image

@montyvesselinov
Copy link

montyvesselinov commented Mar 5, 2024 via email

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 a pull request may close this issue.

3 participants