search

GLVis / glvis

Lightweight OpenGL tool for accurate and flexible finite element visualization
http://glvis.org
BSD 3-Clause "New" or "Revised" License
249 stars 51 forks source link

CI misc improvements #296

Closed justinlaughlin closed 3 weeks ago

justinlaughlin commented 1 month ago

Changes:

tzanio commented 1 month ago

Should we add this to the checklist in https://github.com/GLVis/glvis/pull/284?

justinlaughlin commented 1 month ago

I don't think its necessary since it is only features to help with development. I think the other PRs are very close / ready so I don't want to hold up the release. This could be merged in afterwards.

najlkin commented 1 month ago

You may just rerun it, the Mac runners are extra crappy 😄 🤭

justinlaughlin commented 1 month ago

Btw, has anyone seen this error before? I definitely saw it in a test last week (trying to find the job) but in the next test it was fixed and I'm not sure why.

6: *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'API misuse: setting the main menu on a non-main thread. Main menu contents should only be modified from the main thread.'
najlkin commented 1 month ago

It happens randomly, I have seen that before. It is only on Mac 🍎🪱

justinlaughlin commented 1 month ago

You may just rerun it, the Mac runners are extra crappy 😄 🤭

oh, are you're telling me the mac tests are stochastic? 🤣

we should change the tests results to a probability density function instead of [PASS] / [FAIL]

edit: I was half-joking but out of 6 samples so far these are the {failed tests}: {6,10}, {9}, {10}, {10}, {10,20}, {} I think this is a common issue though https://github.com/actions/runner-images/issues/8642

edit2: I don't see any differences in the runner image between fails/success 🤔... Best solution might just be to re-run a couple of times on failure.

Current runner version: '2.317.0'
Operating System
  macOS
  14.5
  [2](https://github.com/GLVis/glvis/actions/runs/10067769382/job/27832593078?pr=296#step:1:2)3F79
Runner Image
  Image: macos-1[4](https://github.com/GLVis/glvis/actions/runs/10067769382/job/27832593078?pr=296#step:1:4)-arm64
  Version: 20240714.2
  Included Software: https://github.com/actions/runner-images/blob/macos-14-arm[6](https://github.com/GLVis/glvis/actions/runs/10067769382/job/27832593078?pr=296#step:1:7)4/20240714.2/images/macos/macos-14-arm64-Readme.md
  Image Release: https://github.com/actions/runner-images/releases/tag/macos-14-arm64%2F20240[7](https://github.com/GLVis/glvis/actions/runs/10067769382/job/27832593078?pr=296#step:1:8)14.2

edit3: @najlkin I noticed that runs that were using a cache instead of building mfem on macos failed. I tried forcing the mfem-build step for macos and it seems to have worked? We'll see...

edit4: forcing a build did not solve the macos failures😔

justinlaughlin commented 1 month ago

needs a lot of cleanup but the basic idea for image diffs is there. (they are html files in the zip). The reason behind html is so the views can be interactive (via plotly)

e.g. test.diff(zoomed).mfem-logo.html image

synchronized views image

Right now the artifact size is huge because I'm generating it for everything, but in practice it'd only be generated when a test fails.

Still not sure why macos tests are failing 😞

justinlaughlin commented 1 month ago

If anyone wants to try it out, controls are in top-right.

image

najlkin commented 1 month ago

That is really cool 🤩 , but why is it so big? The normal PNGs have like hundreds of KB, while this has multiple MB. Also the UI is a little bit slow with such big files. Is that you store it uncompressed?

najlkin commented 1 month ago

Regarding the failures, it seems this workaround does not work anymore for some reason or in just very stochastic way 😄 https://github.com/GLVis/glvis/blob/master/lib/sdl_main.cpp#L295-L309

najlkin commented 1 month ago

Maybe it is not needed 🤔 , the commit d10bc970ef73e04be0747cedda32450292b16ac3 in PR #174 says it is for version 2.0.12, but the runner uses SDL v2.30.5 😅 . Maybe try to comment it out and see what happens...

najlkin commented 1 month ago

Omg, I maybe know what happens 😮 . It is something else totally. That function GetMainThread() is sometimes called from a non-main thread before it is in the main one, that is what it is complaining about, because constructor of SdlMainThread calls SDL_Init(), which fails then.

justinlaughlin commented 1 month ago

That is really cool 🤩 , but why is it so big? The normal PNGs have like hundreds of KB, while this has multiple MB. Also the UI is a little bit slow with such big files. Is that you store it uncompressed?

It is html for interactivity - I'm not sure how to reduce the file-size. We could just store the diff as a png as well, but I think it is helpful to directly compare all three images (baseline, output, diff). I'm not too concerned about size because in a normal use-case only one or a few of these should be generated.

Omg, I maybe know what happens 😮 . It is something else totally. That function GetMainThread() is sometimes called from a non-main thread before it is in the main one, that is what it is complaining about, because constructor of SdlMainThread calls SDL_Init(), which fails then.

That is a great find 😄

najlkin commented 1 month ago

I mean HTML is fine, but can that HTML use PNG with lossless compression?

najlkin commented 1 month ago

Yeeh, we are passing 🥳 Ok, I will make a PR for it :wink:. (edit: see #298 )

justinlaughlin commented 1 month ago

I mean HTML is fine, but can that HTML use PNG with lossless compression?

maybe? 🤷 It is a plotly.graph_object.Image, ref, lines; I don't see options for compression. I haven't dug too far into it but I think another reason why the html is large is because it is also storing the plotly js source code.

Yeeh, we are passing 🥳 Ok, I will make a PR for it 😉.

🎉 Thank you!

najlkin commented 1 month ago

How about source parameter of plotly.graph_objects.Image, so just storing PNGs and then generating the HTML for interactive plotting? If you include some switch for the files, there could be just a single HTML, so no duplicity of the plotly JS source 😉 .

justinlaughlin commented 1 month ago

How about source parameter of plotly.graph_objects.Image, so just storing PNGs and then generating the HTML for interactive plotting? If you include some switch for the files, there could be just a single HTML, so no duplicity of the plotly JS source 😉 .

Sounds complicated 😅 if this was a core feature I would be more willing to optimize but this is just a matter of saving some MB on failed tests artifacts 🤷. Also the end-goal is to make the CI output easier to interpret/use not harder 😆.

najlkin commented 1 month ago

I think having PNGs around is useful anyway and the easiest thing to interpret 🙂 . That HTML can be just a cherry on top 🍒 and if it is hard to add the switch then just generate a separate one for every image, that would not be a disaster 😉 .

justinlaughlin commented 1 month ago

I think having PNGs around is useful anyway and the easiest thing to interpret 🙂 . That HTML can be just a cherry on top 🍒 and if it is hard to add the switch then just generate a separate one for every image, that would not be a disaster 😉 .

yes agreed :). All the PNGs will still be there, this *diff.html will only show up when a test fails so it is easier to troubleshoot why.

najlkin commented 1 month ago

with the diff loaded from a PNG as well, right? 😛

justinlaughlin commented 1 month ago

How about source parameter of plotly.graph_objects.Image, so just storing PNGs and then generating the HTML for interactive plotting? If you include some switch for the files, there could be just a single HTML, so no duplicity of the plotly JS source 😉 .

oops my bad I don't know why when I first read this I interpreted it differently, this seems pretty straightforward. will check if it reduces size

justinlaughlin commented 1 month ago

How about source parameter of plotly.graph_objects.Image, so just storing PNGs and then generating the HTML for interactive plotting? If you include some switch for the files, there could be just a single HTML, so no duplicity of the plotly JS source 😉 .

Thanks for the idea! Works great :) filesize is much smaller now (still MBs but not tens of MBs)

najlkin commented 1 month ago

So is the diff PNG as well? Can we see it somewhere? I am curious 🤤 Besides that, there seems to be like a top level . directory in the tar, could you remove that? 🤔

najlkin commented 1 month ago

Some filename error, it could not find files to upload 😕

najlkin commented 1 month ago

There is no HTML 😢

justinlaughlin commented 1 month ago

There is no HTML 😢

I had it generate for all cases earlier for testing, but now I set it only to generate if there is a test failure. Filesize is still ~5MB per html. Do you think we should just include it?

najlkin commented 1 month ago

Just for testing, or maybe just attach an example file here so we can see it 😉

najlkin commented 1 month ago

Still no HTML, it was not that bad idea to test it 😄

justinlaughlin commented 1 month ago

Just for testing, or maybe just attach an example file here so we can see it 😉

I changed the glvis/data hash to make an artifact, I'll switch it back later. There is an html file under mesh-explorer

image

image

najlkin commented 1 month ago

Ahaa, like that, but why is it so big? It seems it still includes the picture data. I unzippzed only the HTML and it shows the pictures without the PNGs 😅

justinlaughlin commented 1 month ago

Ahaa, like that, but why is it so big? It seems it still includes the picture data. I unzippzed only the HTML and it shows the pictures without the PNGs 😅

It is the bundled source code. There is an option under figure.write_html() called include_plotlyjs='cdn'. That reduces the filesize down to around the size of the images - but then the html can only be used online. We could do that?

najlkin commented 1 month ago

No, I meant to use file://test.nominal....png in source, so no data in HTML. It is not possible?

justinlaughlin commented 1 month ago

No, I meant to use file://test.nominal....png in source, so no data in HTML. It is not possible?

Can we discuss over slack real quick? might be easier

najlkin commented 1 month ago

Maybe we could add here the that enabling of the style checks for PR CIs. Or separate? 🤔

justinlaughlin commented 1 month ago

Maybe we could add here the that enabling of the style checks for PR CIs. Or separate? 🤔

If you want to add it, up to you. Otherwise imo, separate PR. I need to work on another project so if we want to include this in 4.3 I say ship it as-is. I did consider that too

tzanio commented 3 weeks ago

I just merged master in here, and everything looks good.

Are we ready to merge this?