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

CI fixes #4974

Merged
merged 6 commits into from
Aug 28, 2024
Merged

CI fixes #4974

merged 6 commits into from
Aug 28, 2024

Conversation

penelopeysm
Copy link
Contributor

@penelopeysm penelopeysm commented Aug 23, 2024

Description

Attribution

Things to consider

  • Does it work on log scales?
  • Does it work in layouts?
  • Does it work in recipes?
  • Does it work with multiple series in one call?
  • PR includes or updates tests?
  • PR includes or updates documentation?

Behaviour was changed such that ã becomes \tilde{a}, instead of
\textnormal{\~{a}}.
@penelopeysm penelopeysm changed the base branch from v2 to master August 23, 2024 18:58
@penelopeysm penelopeysm marked this pull request as ready for review August 23, 2024 18:59
Copy link

codecov bot commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.75%. Comparing base (fa65e7d) to head (3873ecc).
Report is 23 commits behind head on master.

Files Patch % Lines
src/backends/plotlybase.jl 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4974      +/-   ##
==========================================
- Coverage   89.77%   89.75%   -0.02%     
==========================================
  Files          40       40              
  Lines        8780     8817      +37     
==========================================
+ Hits         7882     7914      +32     
- Misses        898      903       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@penelopeysm
Copy link
Contributor Author

penelopeysm commented Aug 23, 2024

The ubuntu/macOS + Julia 1.6 tests are failing because of image comparison regression tests, specifically on image number 6 with GR backend.

Looking across a bunch of logs, it seems that any runs with GR.jl v0.73.x cause this test to fail, whereas v0.72.x works.

I've extracted the images from the GitHub runner, for good measure:

ubuntu-latest, 1.6

Difference: 0.003957361095933401 tolerance: 0.0005

gr_6_ubuntu-latest_1 6

macOS-13, 1.6

Difference: 0.003957361095933401 tolerance: 0.001

gr_6_macos-13_1 6

Reference image

Taken from https://github.com/JuliaPlots/PlotReferenceImages.jl/blob/master/Plots/gr/1.36.4/ref006.png

ref006

@penelopeysm
Copy link
Contributor Author

@BeastyBlacksmith, pinging you because you sorted my other PR, I hope that is ok:)

I think this might be the extent to which I feel confident in fixing stuff. The version clash on GR (comment above, Column 2 below) is a bit bizarre; I've tried to track down why it resolves to 0.72.8 sometimes and 0.73.7 other times, but without any success.

If we stick a GR = 0.73 compat entry (Column 3), it will successfully resolve to 0.73.7 on all combinations of OS/Julia versions and we can update the image regression test, but that will obviously break backwards compatibility. We also can't just manually install [email protected] while running tests because of this error.

The Windows + Julia 1.6 tests currently time out because of Kaleido. It could be made to run by bumping the compat on PlotlyKaleido to 1,2. However, that in turn causes weird fluctuations in the resolved version of GR (Column 4).

(If we update compat for both deps (Column 5), and also update the image regression test, then I think all the tests should pass.)

OS/Julia version With current compat bounds With GR=0.73 With PlotlyKaleido=1,2 With both
1.6, Ubuntu 0.73.7 0.73.7 0.73.7 0.73.7
1.6, Windows hangs hangs 0.73.7 0.73.7
1.6, macOS-13 0.73.7 0.73.7 0.73.7 0.73.7
1.7, Ubuntu 0.72.8 0.73.7 0.73.7 0.73.7
1.8, Ubuntu 0.72.8 0.73.7 0.72.8 0.73.7
1.9, Ubuntu 0.72.8 0.73.7 0.72.8 0.73.7
1.10, Ubuntu 0.72.8 0.73.7 0.73.7 0.73.7
1.10, Windows 0.72.8 0.73.7 0.73.7 0.73.7
1.10, macOS 0.72.8 0.73.7 0.73.7 0.73.7
pre, Ubuntu 0.72.8 0.73.7 0.72.8 0.73.7

@BeastyBlacksmith
Copy link
Member

BeastyBlacksmith commented Aug 26, 2024

Thank you for taking a look into this.

On GR: given that the reference clearly has artifacts that got fixed it seems reasonable to me to set the compat to 0.73 and update the refrence image.

On PlotlyKaleido: This ones a bit more work. Since, to be compatible with PlotlyKaleido v2 we need to check in the savefig methods for all outputs that use kaleido (prob. everything except for html) if the kaleido server is running and start it before calling the PlotlyKaleido savefig function.
See JuliaPlots/PlotlyKaleido.jl#6

@BeastyBlacksmith BeastyBlacksmith merged commit 288372d into JuliaPlots:master Aug 28, 2024
13 of 16 checks passed
@BeastyBlacksmith
Copy link
Member

@penelopeysm would you be willing to do a v2 port of this?

@penelopeysm penelopeysm deleted the ci-fixes branch August 29, 2024 23:42
@penelopeysm
Copy link
Contributor Author

Happy to do that! 🙂

@penelopeysm penelopeysm mentioned this pull request Sep 8, 2024
7 tasks
BeastyBlacksmith added a commit that referenced this pull request Sep 10, 2024
* CI fixes (#4974)

* Update tests for Latexify 0.16.5

Behaviour was changed such that ã becomes \tilde{a}, instead of
\textnormal{\~{a}}.

* Test Julia 1.6 on x86 macOS instead of ARM

See: https://discourse.julialang.org/t/how-to-fix-github-actions-ci-failures-with-julia-1-6-or-1-7-on-macos-latest-and-macos-14/117019

* Bump GR compat and Plots version number

* update plotly show methods for PlotlyKaleido v2

* remove Pkg

* add it back

---------

Co-authored-by: Simon Christ <[email protected]>

* Don't need to edit CI for 1.6 as no longer supported

* Don't test GR/50 on macOS

* Reorganise failing tests

 - Disabled reference tests 25 and 30, which require StatsPlots support,
   for all backends (in PlotsBase/src/examples.jl)
 - Separate tests that are skipped and tests that are known to be broken
   due to upstream issues
 - Re-enable reference test 41 as upstream issue has been fixed
   (JuliaLang/julia#47261)
 - Disable reference test 50 because of upstream issue
   (jheinen/GR.jl#550)

---------

Co-authored-by: Simon Christ <[email protected]>
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.

2 participants