-
-
Notifications
You must be signed in to change notification settings - Fork 74
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: standardise workflows using SciML's reusable workflows #808
base: master
Are you sure you want to change the base?
Conversation
Thanks. We can add these with Catalyst 14, unfortunately merging now would lead to broken dev docs so this has to wait till that is merged to master. |
I'm confused, why do we have to wait? The documentation workflows haven't been standardised (yet), so remain unaffected by this. Aren't these mutually exclusive? |
37f8c93
to
8122359
Compare
The seems to be some errors in the runtests that are not there on normal CI runs? Is this something that is related to this PR, or a normal MTK issue that for some reason only does things for this PR? |
|
Will |
@thazhemadam we've updated everything but docs and CI -- we are now frequently getting test failures due to codecov token limits. Is that fixed in the SciML wide versions of those scripts? I ended up having to disable test failures for both with regards to codecov to keep tests passing consistently here. If it is fixed in the SciML-wide versions we can switch, otherwise we'd want to keep versions here that turn off failures from codecov. (If your scripts can be fixed to do that I am happy to merge such versions.) Thanks! |
The codecov token limit issue is something that severely affected SciML's CI setups as a whole, so having that addressed appropriately in the centralized solution has always been important. |
* Standardise the tests workflow, using the reusab SciML tests workflow. * Rename the tests workflow to `Tests.yml`, instead of `CI.yml`, since it's more semantically correct and CI encompasses all the workflows that are run.
8122359
to
4ec7718
Compare
We need the failing CI based on codecov flag to be false as otherwise we get too many failures induced by codecov problems (they seem to go down / break quite frequently). |
Anant's point is that's fixed by the changes here. It's fixed because that failure only happens on forks, and so it's simply disabled for forks. |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
@giordano do you know what you'd do to these? https://github.com/SciML/Catalyst.jl/blob/at/reusable-ci-workflows/src/graphs.jl#L146-L152 |
I don't see |
The construction of the Cmd object is the main thing, it doesn't matter how you use it, whether in run, open, or else. |
@isaacsas can you help? I can't parse those docs at all. |
I can't make suggestions and I'm away from keyboard, so I can't produce a diff, but basically you need to use open(`$(fun()) -T$format`, io, write = true) do gv
pprint(gv, graph)
end and remove the wrapping |
Side note, the nice way to override jll products is https://docs.binarybuilder.org/stable/jll/#Overriding-specific-products, instead of doing getfield magics Line 144 in c7c204b
|
At some point we should probably just investigate if we can use GraphMakie or such and stop using the binaries. |
I commented the test for now since I'm not sure how to handle this, and I'll see if all of the others are done. |
Ok, but I’d prefer to hold off merging this until we know how to fix graph plotting. I’m away from my computer till Monday but can try to investigate some next week when I am back from vacation. |
The HomotopyContinuation needs JuliaHomotopyContinuation/HomotopyContinuation.jl#579 |
Yes that's my intention, I just wanted to solve everything else first. |
JuliaHomotopyContinuation/HomotopyContinuation.jl#580 is the next one. |
@ChrisRackauckas are deprecation warnings supposed to completely fail CI? Is that a new behavior? |
Yes, we are trying to enforce it globally, which of course at first is going to be tough 😅 |
Is there a way to set it to only fail for warnings generated in the Catalyst, and not libraries it depends on? I think having to handle ensuring libraries we depend on via extensions or tests also handle deprecation warnings is potentially going to be difficult / burdensome for us. |
There is not. But, I will make the bold statement that since we are putting this on everywhere, it should not be that difficult to maintain it in Catalyst. In the end, it should make maintaining Catalyst easier because it shifts the burden of updating downstream to the person who makes the deprecation. Currently, making a deprecation is "free", which incentivizes making a change that deprecates and then letting it be someone else's problem 2 years down the line. With this change, you have to update downstream because otherwise you broke tests. The only issues then are the libraries outside of SciML which are less strict. But Catalyst mostly just hits dependencies that the rest of SciML uses, so it should get the treatment before it's noticed here anyways. And in all of the SciML updates, so far only HomotopyContinuation and Turing have turned out to be issues, so it seems the set is sufficiently policeable. So I'd like to give it a try at least. If the experiment fails, we can flip a switch in just a single .github file and it would go away, so it's easy to change after this canonicalization. |
We can test it out sure. I think the packages we'll have to worry about are BifurcationKit, Makie, Plots, HomotopyContinuation, DynamicPolynomials, and DynamicQuantities. |
And Unitful. |
Current battle: saschatimme/MixedSubdivisions.jl#24 |
This was my worry. I'm not confident that even if we get all deps to have no depwarnings at this point in time they will stay passing (because, for example, their CI won't fail over depwarns so they won't notice sudden depwarns in their tests). |
Well we have many years worth of ignoring compats so right now it's the worst. But I plan to keep on top of it. At least right now it doesn't seem like too bad of an idea... we'll see how it feels 2 years down the line 😅 |
Update the workflows in this repository to use SciML's reusable workflows.
This is part of a larger effort to standardise the SciML's CI workflows for more generic and common requirements, to keep the workflows uniform and easier to maintain.