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

Create invalidations.yml #26

Merged
merged 1 commit into from
Aug 31, 2022
Merged

Create invalidations.yml #26

merged 1 commit into from
Aug 31, 2022

Conversation

ranocha
Copy link
Member

@ranocha ranocha commented Aug 30, 2022

@devmotion
Copy link
Member

Hmm, I don't think it's useful in this package? It's very very stable, so it's very unlikely to be changed significantly, it has no dependencies, and it does not overload any existing function in base.

@ranocha
Copy link
Member Author

ranocha commented Aug 30, 2022

I would ague the other way round: If this package is so stable, Ci will basically never run, so it's not a problem to include this setup. On the other hand, if this package introduced invalidations for some reason, we would probably want to know about it.

@devmotion
Copy link
Member

But why include something if it's never run? 😅 It's also still unclear to me how one could introduce any invalidations in this package.

So my impression is that probably it is much much more relevant to check for invalidations in other (probably also SciML) packages.

@ranocha
Copy link
Member Author

ranocha commented Aug 30, 2022

Does it cost anything to include it? If not, why shouldn't we include it?

@devmotion
Copy link
Member

It will run every time a simple PR (such as #27, a docstring update, or some reformatting) is opened, I don't think that's useful.

@ChrisRackauckas
Copy link
Member

This cost is the least of our problems. I'll always trade CI time to decrease human time.

@ChrisRackauckas ChrisRackauckas merged commit 5fe6013 into SciML:master Aug 31, 2022
@devmotion
Copy link
Member

Fair enough. I'm still curious though, @ranocha, why you opened this PR to MuladdMacro instead of more active packages that implement methods from base and are also more difficult/impossible to check manually such as SciMLBase, DiffEqBase, OrdinaryDiffEq etc.? Did you see any problems with MuladdMacro that we should fix?

@ranocha
Copy link
Member Author

ranocha commented Aug 31, 2022

I didn't only do it for MudallMacro.jl but also for other packages such as Static.jl. I think we should get started with the most baseline packages and then extend it step by step to higher-level packages.
I did this since I spend two days hunting and fixing invalidations, which is definitely not fun.

@ranocha
Copy link
Member Author

ranocha commented Aug 31, 2022

For example, OrdinaryDiffEq.jl pulls in so many packages that we should get started with all of these dependencies first, I think.

@ChrisRackauckas
Copy link
Member

I think someone should just do a mass PR to all of SciML adding this. There's some tool for doing mass PRs like that, and I'd accept it (though please add [ci skip] if that's done 😅)

@ranocha ranocha deleted the patch-1 branch August 31, 2022 13:48
@ranocha
Copy link
Member Author

ranocha commented Aug 31, 2022

Are the main branches all master in SciML or do some have main, others master?

@ChrisRackauckas
Copy link
Member

A few of the newer ones probably have main 😅

@ranocha
Copy link
Member Author

ranocha commented Aug 31, 2022

https://github.com/julia-actions/MassInstallAction.jl does not seem to have an option for this. Are we okay with checking the main branch by hand? Or shall I try to find something else?

@ChrisRackauckas
Copy link
Member

Yeah that sounds fine.

@ranocha
Copy link
Member Author

ranocha commented Aug 31, 2022

Looks like they also have no simple way of editing the commit message to include [ci skip]. Let me have a look at it

@ChrisRackauckas
Copy link
Member

Well today I'm flying, so there's no better day to slam CI anyways 😅.

@ranocha
Copy link
Member Author

ranocha commented Aug 31, 2022

Yeah, but MassInstallAction.jl does not work right now (on Julia v1.8), see julia-actions/MassInstallAction.jl#57

@ChrisRackauckas
Copy link
Member

Ouch.

@ranocha
Copy link
Member Author

ranocha commented Aug 31, 2022

Okay, fixed locally (and submitted PRs). I will run

wget https://raw.githubusercontent.com/SciML/MuladdMacro.jl/master/.github/workflows/invalidations.yml
export MY_GITHUB_TOKEN=""
julia
julia> using Pkg; Pkg.activate(temp=true); Pkg.develop("MassInstallAction"); using MassInstallAction

julia> workflow = MassInstallAction.Workflow("Invalidations", "invalidations.yml" => read("invalidations.yml", String))

julia> MassInstallAction.install(workflow,
                                 "SciML";
                                 token = ENV["MY_GITHUB_TOKEN"],
                                 cc = ["ranocha", "ChrisRackauckas"],
                                 commit_message = "MassInstallAction: Install the Invalidations workflow on this repository (automated commit made by MassInstallAction.jl) [ci skip]",
                                 pkg_url_type = :ssh)

Okay?

@ChrisRackauckas
Copy link
Member

That sounds great.

@ranocha
Copy link
Member Author

ranocha commented Aug 31, 2022

I get something like

[massinstallaction/set-up-Invalidations c862fdc] MassInstallAction: Install the Invalidations workflow on this repository (automated commit made by MassInstallAction.jl) [ci skip]
 1 file changed, 33 insertions(+)
 create mode 100644 .github/workflows/invalidations.yml
ERROR: Permission to SciML/DEDataArrays.jl.git denied to ranocha.
fatal: Could not read from remote repository.

for quite a few packages. I guess someone with more rights needs to repeat this. I used my branch https://github.com/ranocha/MassInstallAction.jl/tree/hr/tmp of MassInstallAction.jl for this, bundling my recent PRs to this package.

@devmotion
Copy link
Member

It seems the PRs don't skip CI?

@ranocha
Copy link
Member Author

ranocha commented Aug 31, 2022

I included [ci skip] in the commit messages - what else can we do?

I tested it before for my own repos, e.g.,

There, all GitHub actions CI were skipped.

@devmotion
Copy link
Member

devmotion commented Aug 31, 2022

It seems it's actually only a problem with some of the buildkite pipeline configurations. Eg in DiffEqBase buildkite was run (but nothing else it seems?) since it is only skipped if the commit message (?) includes [skip tests]: https://github.com/SciML/DiffEqBase.jl/blob/master/.buildkite/pipeline.yml Not sure if that setup is intentional.

@devmotion
Copy link
Member

Same in OrdinaryDiffEq, so probably should include [skip tests] next time. Not sure if that also would skip Github actions, or if it is needed in addition.

quinnj pushed a commit to JuliaStrings/InlineStrings.jl that referenced this pull request Sep 5, 2022
quinnj pushed a commit to JuliaWeb/HTTP.jl that referenced this pull request Sep 5, 2022
hyrodium pushed a commit to JuliaGeometry/Rotations.jl that referenced this pull request Sep 8, 2022
j-fu pushed a commit to WIAS-PDELib/GridVisualize.jl that referenced this pull request Sep 9, 2022
JonasIsensee pushed a commit to JuliaIO/JLD2.jl that referenced this pull request Sep 10, 2022
ranocha added a commit to ranocha/GeometryBasics.jl that referenced this pull request Sep 15, 2022
sethaxen pushed a commit to JuliaGeometry/Octonions.jl that referenced this pull request Sep 19, 2022
j-fu pushed a commit to JuliaGeometry/Triangulate.jl that referenced this pull request Sep 30, 2022
jverzani pushed a commit to JuliaMath/Roots.jl that referenced this pull request Oct 1, 2022
ranocha added a commit to ranocha/OrderedCollections.jl that referenced this pull request Oct 6, 2022
ranocha added a commit to ranocha/PrettyTables.jl that referenced this pull request Oct 13, 2022
ronisbr pushed a commit to ronisbr/PrettyTables.jl that referenced this pull request Oct 13, 2022
giordano pushed a commit to JuliaPackaging/Preferences.jl that referenced this pull request Nov 3, 2022
ranocha added a commit to trixi-framework/Trixi2Vtk.jl that referenced this pull request Nov 26, 2022
* Create Invalidations.yml

This is based on https://github.com/julia-actions/julia-invalidations. Adding such checks came up in https://discourse.julialang.org/t/potential-performance-regressions-in-julia-1-8-for-special-un-precompiled-type-dispatches-and-how-to-fix-them/86359. I suggest to add this check here since this package is widely used as a dependency.

See also SciML/MuladdMacro.jl#26 and SciML/MuladdMacro.jl#29

* Update Invalidations.yml

Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
oschulz pushed a commit to JuliaArrays/ElasticArrays.jl that referenced this pull request Nov 29, 2022
SaschaMann pushed a commit to julia-actions/julia-invalidations that referenced this pull request Dec 6, 2022
This is based on some discussion we recently had in SciML, see SciML/MuladdMacro.jl#26 and SciML/MuladdMacro.jl#29
mcabbott pushed a commit to JuliaDiff/ForwardDiff.jl that referenced this pull request Dec 10, 2022
ararslan pushed a commit to JuliaIO/FileIO.jl that referenced this pull request Dec 16, 2022
timholy pushed a commit to JuliaGraphics/Colors.jl that referenced this pull request Jan 4, 2023
simonbyrne pushed a commit to JuliaMath/Quadmath.jl that referenced this pull request May 17, 2023
jw3126 pushed a commit to jw3126/ArgCheck.jl that referenced this pull request Sep 23, 2023
ScottPJones pushed a commit to JuliaString/Format.jl that referenced this pull request Feb 20, 2024
ScottPJones added a commit to JuliaString/Format.jl that referenced this pull request Feb 20, 2024
giordano added a commit to JuliaMath/QuadGK.jl that referenced this pull request May 10, 2024
* Create Invalidations.yml

This is based on https://github.com/julia-actions/julia-invalidations. Adding such checks came up in https://discourse.julialang.org/t/potential-performance-regressions-in-julia-1-8-for-special-un-precompiled-type-dispatches-and-how-to-fix-them/86359. I suggest to add this check here since this package is widely used as a dependency.

See also SciML/MuladdMacro.jl#26 and SciML/MuladdMacro.jl#29

* Apply suggestions from code review

---------

Co-authored-by: Mosè Giordano <[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.

3 participants