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

DUMMY: investigating Windows precompilation failure in 1.8.2 #4445

Closed
wants to merge 22 commits into from

Conversation

BioTurboNick
Copy link
Member

@BioTurboNick BioTurboNick commented Oct 7, 2022

I was curious about why Plots had an issue precompiling on Windows in JuliaLang/julia#46989, and I haven't been able to reproduce the issue on my system. So I'm trying to revert the workaround to see if the problem remains in the CI.

@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Base: 80.63% // Head: 80.66% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (cb85cf6) compared to base (c128fc3).
Patch coverage: 45.16% of modified lines in pull request are covered.

❗ Current head cb85cf6 differs from pull request most recent head 8ce6fdc. Consider uploading reports for the commit 8ce6fdc to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4445      +/-   ##
==========================================
+ Coverage   80.63%   80.66%   +0.02%     
==========================================
  Files          30       30              
  Lines        7467     7494      +27     
==========================================
+ Hits         6021     6045      +24     
- Misses       1446     1449       +3     
Impacted Files Coverage Δ
src/precompilation.jl 0.00% <0.00%> (ø)
src/backends/gr.jl 88.02% <50.00%> (-0.87%) ⬇️
src/recipes.jl 68.32% <0.00%> (+0.07%) ⬆️
src/components.jl 89.40% <0.00%> (+2.58%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@t-bltg
Copy link
Member

t-bltg commented Oct 7, 2022

As I recall, it appears to produce the same error.

@BioTurboNick
Copy link
Member Author

BioTurboNick commented Oct 7, 2022

Nearest I can figure is that GR is failing to create the initial temporary file in _show (or at least, not in the expected location), which Plots then tries to read from to copy over to the intended permanent file.

Why it only seems to happen in CI so far, unclear. I'll keep poking.

EDIT: Yep, if I deliberately create an empty file at filepath after the call to gr_display, it precompiles correctly in CI.

Found a change in base/stream.jl between 1.8.1 and 1.8.2 - checking that - did not help

LibUV was bumped from 1.42.0 to 1.44.2 between 1.8.1 and 1.8.2
LibUV_jll was bumped from 2.0.1+5 to 2.0.1+11
Apart from that, some changes in the bowels of Julia's C code.

@BioTurboNick
Copy link
Member Author

Irony: Using tmate to debug it on the CI machine, but it doesn't support scrolling, and the stack trace is too damn long 😆. And sadly resistant to my AbbreviatedStackTraces.

@BioTurboNick
Copy link
Member Author

Either GR is failing to create the file, perhaps via some change in libuv?, or it's being deleted before control returns to Plots. And unclear how precompilation environment differs from the normal REPL execution, where things work fine.

Since LibUV is tightly bound to the Julia version, I don't think it'll be possible to debug that here. So next is to look at GR.

Inquiring if there's an easy way to get debug information out of GR: sciapp/gr#161

Next obvious question, perhaps, is why this error doesn't show up in GR.jl's CI?

Other question is how far back in Plots does this problem happen? The failure first appears in Plots v.1.34.0 - when this whole method of precompilation was introduced.

@BioTurboNick
Copy link
Member Author

BioTurboNick commented Oct 8, 2022

GR.savefig works fine in the same environment.

I'm not sure how to reconcile the different approaches Plots takes to saving files via GR and how GR does it, to determine what might be going wrong in Plots'. Is it possible Plots is doing something not intended by GR?

@t-bltg
Copy link
Member

t-bltg commented Oct 8, 2022

I agree that reproducing the issue in plain GR might help narrow the issue.

@BioTurboNick
Copy link
Member Author

BioTurboNick commented Oct 8, 2022

I think I've nailed it. EDIT: Nope.

The width and height of the viewport are too small. 1024x768 leads to failure to render and save the file.

If I force display_width_px and display_height_px to be the values I observed from precompilation on the CI machine, I can reproduce the failure locally.

Plots.jl/src/backends/gr.jl

Lines 671 to 672 in e9fb3b3

display_width_meters, display_height_meters, display_width_px, display_height_px =
GR.inqdspsize()

Three things to act on:
1) is to make this code robust to small display port sizes
2) GR should presumably produce an error when it is unable to write to the file?
3) Revert the workaround for precompilation

Why did Julia 1.8.2 trigger it though? Idk.

Crap. Replaced the wrong values. But got the same error. But it does suggest that GR is silently failing internally.

@t-bltg
Copy link
Member

t-bltg commented Oct 9, 2022

But it does suggest that GR is silently failing internally.

The does show the limit of having a c backend, and not a pure julia one.
Until we have something approaching that (or have sciapp/gr low level debugging tools), we are forced to cope with nasty bugs like this one.

@mkitti
Copy link
Contributor

mkitti commented Jan 4, 2023

Did this ever come to any conclusion? I've been thinking of taking a look.

@BioTurboNick
Copy link
Member Author

Did this ever come to any conclusion? I've been thinking of taking a look.

I don't believe so. I didn't want to mess with GR so I stopped there.

@t-bltg
Copy link
Member

t-bltg commented Jan 12, 2023

Superseded by #4617.

@t-bltg t-bltg closed this Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants