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

add nightly test to CI #2024

Closed
wants to merge 3 commits into from
Closed

add nightly test to CI #2024

wants to merge 3 commits into from

Conversation

Moelf
Copy link
Contributor

@Moelf Moelf commented Jun 6, 2022

No description provided.

@MakieBot
Copy link
Collaborator

MakieBot commented Jun 6, 2022

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt.

using time

master:  10.98 < 11.08 > 11.15, 0.06+-
pr:      11.01 < 11.11 > 11.36, 0.12+-
speedup: 0.98 < 1.00 > 1.01, 0.01+-
median:  +0.33% => invariant

This PR does not change the using time.

ttfp time

master   29.90 < 30.19 > 30.87, 0.33+-
pr       29.76 < 30.12 > 30.64, 0.32+-
speedup: 0.99 < 1.00 > 1.01, 0.01+-
median:  -0.21% => invariant

This PR does not change the ttfp time.

@SimonDanisch
Copy link
Member

I kind of want to keep CI runs to a minimum... And I also don't want to fix Julia nightly bugs in Makie ;)
Is there any really important reason besides that to test on nightly, that would justify this?

@Moelf
Copy link
Contributor Author

Moelf commented Jun 6, 2022

For one, 1.8 is about to release and AFAIC Makie doesn't work on 1.8?

We need CI so Makie is ready day 1 of new releases

@SimonDanisch
Copy link
Member

We need CI so Makie is ready day 1 of new releases

Well, it won't be magically ready just by having a CI ;)

@SimonDanisch
Copy link
Member

Also, I got convinced to use 1 now for CI, which should switch us to 1.8 once it's released.

@Moelf
Copy link
Contributor Author

Moelf commented Jun 6, 2022

Yeah but when it is released, Makie will be broken for a long time right?

Having CI allows us to fix things before the public release

@SimonDanisch
Copy link
Member

Well, but I don't think anyone here will debug nightly bugs... Especially since Julia is not supposed to break packages with releases < 2.0, so they almost by definition shouldn't be anything that's fixable in Makie itself.

@greimel
Copy link
Contributor

greimel commented Jun 6, 2022

This proposal will not help, because nightly would test on Julia 1.9-dev, not 1.8-rc.

What you could do is "^1.8.0-0". This though might be too cumbersome because CI scripts need to be updated regularly.

Hopefully it will become easier to test on pre-releases in the future. See the proposal here julia-actions/setup-julia#84 to add latest-prerelease analogous to nightly for usage in CI.

@jkrumbiegel
Copy link
Member

Don't they run tests across the ecosystem anyway to screen for changes that might break packages?

@Moelf
Copy link
Contributor Author

Moelf commented Jun 6, 2022

But before 1.8 is frozen, we would have fixed the bugs, once it's frozen, it's not gonna break more stuff usually

@Moelf
Copy link
Contributor Author

Moelf commented Jun 6, 2022

Actually if we add 1.8, it would work apparently, using rc, we should at least test rc right?

@Moelf
Copy link
Contributor Author

Moelf commented Jun 6, 2022

Especially since Julia is not supposed to break packages with releases < 2.0

assuming none of Makie or any recursively dependent upon packages use any non-public API

@giordano
Copy link

giordano commented Jun 6, 2022

Don't they run tests across the ecosystem anyway to screen for changes that might break packages?

You may be hooking into internals of julia which have been intentionally broken, and no one will tell you (I think they used to, but not anymore). So you're better off actually running tests on nightly.

@jkrumbiegel
Copy link
Member

jkrumbiegel commented Jun 6, 2022

I think this might be more of a psychological thing. We don't want the CI to return failure status for normal PRs if the only reason is that there is a yet unfixed incompatibility with the Julia release candidate. As we can't really communicate well on the overview page what the exact reason is that something failed and rely on these quick cues of the red cross and green tick while sifting through all the notifications, it might not help us that much. Practically speaking, people try Makie with unreleased versions often enough so that we get a heads up anyway if something fails.

@SimonDanisch
Copy link
Member

Yeah, what @jkrumbiegel says ;)
I just don't want to deal with the CI failing on unrelated PRs, just because nightly/pre-release changed something - it's hard enough already to review CI failures with the current volume of Makie PRs.
If someone critically relies on Makie to be immediately ready once 1.8 gets released, they should invest the work and run the tests etc locally.

@Moelf
Copy link
Contributor Author

Moelf commented Jun 6, 2022

We don't want the CI to return failure status for normal PRs if the only reason is that there is a yet unfixed incompatibility with the Julia release candidate

you can still merge it citing "CI 1.8 fails for unrelated reason"

@SimonDanisch
Copy link
Member

SimonDanisch commented Jun 6, 2022

So... what's the point then? Also, that will be shown in the status overview for CI, which I quite heavily rely on, to quickly see if everything is working.

@Moelf
Copy link
Contributor Author

Moelf commented Jun 6, 2022

So... what's the point then?

so we have a place to see and talk about potential problem on upcoming release?

@SimonDanisch
Copy link
Member

So an unrelated PR is a good place to talk about why nightly is failing?
I think you don't realize how tough it is right now to review all the PRs, and every little problem with CI is really compounding into making it super hard to get the huge volume of PRs reviewed.
As long as nobody as paying me or @jkrumbiegel or anyone else to deal with this, I'm not going to take on that extra work.

@Moelf Moelf deleted the Moelf-patch-1 branch June 6, 2022 10:57
@Moelf
Copy link
Contributor Author

Moelf commented Jun 6, 2022

you can use allow-fail for nightly if you rely on green tick visual aids btw

@SimonDanisch
Copy link
Member

I really appreciate that, but from my experience over the last 8 years or so, I just know that having the CI run pro forma without having resources allocated to fix the failures will just waste CI compute, without really fixing bugs earlier or anything.
Also, as @jkrumbiegel pointed out, from experience bug reports come in fast enough from everyone running pre-releases and nightly locally, so Makie not being ready at release time hasn't been an issue so far.
Btw, I myself use nightly exclusively right now, so there is some soft guarantee, that Makie will run on new versions.

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.

6 participants