Catalyst.jl
Catalyst.jl copied to clipboard
SciML
ci: standardise workflows using SciML's reusable workflows
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.
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.
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? That said, I'm okay to wait to have this merged.
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?
depwarn is set to true with the new centralised workflows, which seems to be the reason why the tests are failing.
This can be configured to be set to false as well, if that's what you'd prefer instead (although it's not recommended).
Will depwarn = true cause tests to fail here even if the warnings are only coming from code called within a library we depend on? (i.e. we aren't directly calling deprecated code, the library we depend on is.)
@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. The centralized solution limits when codecov is run to only when PRs are made from branches that don't originate from forks (which isn't very often). At some point, we'll have codecov run only on the default branch instead.
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.
@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 open, pprint, or write = true in there at all. Is there another page?
The construction of the Cmd object is the main thing, it doesn't matter how you use it, whether in run, open, or else.
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 fun() (which is the same suggestion as above https://github.com/SciML/Catalyst.jl/pull/808#discussion_r1698689952)
Sorry, but I’m not really up on this interfacing to an external binary stuff (which has never really worked that robustly with the jll when I checked). This was just copied from Catlab I believe.
Side note, the nice way to override jll products is https://docs.binarybuilder.org/stable/jll/#Overriding-specific-products, instead of doing getfield magics https://github.com/SciML/Catalyst.jl/blob/c7c204b915f6a484f0b22c35f0833868e5425663/src/graphs.jl#L144
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 https://github.com/JuliaHomotopyContinuation/HomotopyContinuation.jl/pull/579
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.
Yes that's my intention, I just wanted to solve everything else first.
https://github.com/JuliaHomotopyContinuation/HomotopyContinuation.jl/pull/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.
Current battle: https://github.com/saschatimme/MixedSubdivisions.jl/pull/24