search

JuliaAstro / Transits.jl

Flexible photometric transit curves with limb darkening
https://juliaastro.github.io/Transits.jl/dev/
MIT License
20 stars 6 forks source link

Using `Breakage` CI for KeywordCalls #24

Open cscherrer opened 3 years ago

cscherrer commented 3 years ago

In case you haven't seen it, this is really helpful: https://github.com/abelsiqueira/Breakage

It gives you results like this: https://github.com/cscherrer/KeywordCalls.jl/actions/runs/888461853

For this to be really useful, tests in downstream packages (Transits and MeasureTheory) should be set up to confirm that functionality depending on KeywordCalls is working correctly.

Maybe this is already the case? If that's so, this can be closed right away. The hope is just to avoid having to bug you to check downstream functionality manually. Probably not such a big deal, since KeywordCalls is getting pretty stable now (other than some weirdness with default values). But it's still nice to have one more check on stability :)

icweaver commented 3 years ago

Oh that's really neat!

I just realized that I was using our old manual way of constructing objects in our tests instead of with KeywordsCalls. That should be switched over now (just a quick change), is there anything else that we should add on our end for Breakage to pick up on?

cscherrer commented 3 years ago

Yeah, Breakage is really nice. As far as anything else to add... I added some tests in KeywordCalls like this:

@test 0 == @ballocated f(alpha=1,b=2,c=3)

Maybe something like that (but building your objects that use KeywordCalls, not KeywordCalls directly), and one or two with @belapsed and some reasonable threshhold? That would make it easy to check that precompilation doesn't slow things down

icweaver commented 3 years ago

Great call (hah). I still need to add some extra tests to up our coverage in general, but the additional tests checking the memory allocation and elapsed time using KeywordCalls should be up now and look to be working well locally for me! I will be sure to give an update once the registry catches up with your latest changes

icweaver commented 3 years ago

Oh weird, it's not the registry, since cscherrer/cs-function-resolution was already merged before the update, IIUC. For some reason, I seem to be seeing allocations/redefinition warnings in the CI for Julia 1.5+ across platforms, even though KeywordCalls is not doing these things when I test it locally. Will double check the setup I am using

cscherrer commented 3 years ago

Good to know. I had been seeing these some of this in MeasureTheory too. Can you try with this? https://github.com/cscherrer/KeywordCalls.jl/tree/cs-instance-type

It seemed better behaved to me, but I'm not sure it's yet 100%

icweaver commented 3 years ago

hmmm, still seems to have the same issue on the *nix platforms. Will check for Windows next

cscherrer commented 3 years ago

I think I need to learn how to use SnoopCompile. You ever tried it? https://timholy.github.io/SnoopCompile.jl/stable/

icweaver commented 3 years ago

Same issue in Windows as well for Julia 1.5+. Was also able to reproduce locally for Julia 1.5 and nightly on linux. I guess 1.6 was our sweet spot, although it's a little strange that it only fails remotely

icweaver commented 3 years ago

Oh, I haven't, but maybe Miles has?

cscherrer commented 3 years ago

Oh 1.6 is ok? I was seeing trouble on 1.4 and 1.5 for a while now. Maybe I misread your "+"

icweaver commented 3 years ago

Right, I meant v1.5, 1.6, and nightly CI are failing for me, but 1.6 on my local machine seems to be ok at least (Linux Mint 20.1 x86_64). Strangely enough, v1.0 seems to be passing on all platforms

Oh! It is labeled as Julia 1 in the CI, but it is actually installing v1.6! Sorry about the confusion there. So really, it's just v1.5 and nightly that are failing there. At least that is lining up with what I am seeing locally now 😅

cscherrer commented 3 years ago

Oh good! That's consistent with what I was seeing. That's why I have this line in the tests: https://github.com/cscherrer/KeywordCalls.jl/blob/cce1b93b913b92c8419eb52aa6b97e2cd44efa2f/test/runtests.jl#L61

icweaver commented 3 years ago

Oh wow, I did not realize non-allocating was specific to 1.6, adding that in now! Just out of curiosity, what feature does KeywordCalls use to accomplish this?

cscherrer commented 3 years ago

There's not a specific feature. It was just working great in 1.6 but not in earlier versions. I put the conditional in the tests to make sure we keep the 1.6 performance. It would be great is someone finds a way to make this better for 1.4-1.5 without compromising 1.6, but I don't think I'll spend more time on that until there's new information.

icweaver commented 3 years ago

I see, definitely reasonable!