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 option to check if frules and rrules are type-inferrable #78

Merged
merged 11 commits into from
Dec 17, 2020

Conversation

sethaxen
Copy link
Member

@sethaxen sethaxen commented Dec 5, 2020

As discussed with @willtebbutt in JuliaDiff/ChainRules.jl#321 (comment), this PR adds a check_inferred=true default keyword argument to the tester functions to check if the frules, rrules, pullbacks, and thunks are all type-stable.

The functionality is there, but I'm having some difficulty testing that test_scalar fails when an rrule or frule is type-unstable. Perhaps a testing kung fu master could give me a pointer.

@willtebbutt
Copy link
Member

Hmmm I'm stumped by this one -- not a massive Test.jl expert though. Maybe someone on slack or discourse could assist?

@oxinabox
Copy link
Member

oxinabox commented Dec 7, 2020

The functionality is there, but I'm having some difficulty testing that test_scalar fails when an rrule or frule is type-unstable. Perhaps a testing kung fu master could give me a pointer.

We have a scary, but functional tool for this in
https://github.com/JuliaDiff/ChainRulesTestUtils.jl/blob/master/test/meta_testing_tools.jl

Basically do @test fails(()-><<CODE THAT WILL CALLS A TEST FAILURE>>)
you can see some examples in that file

src/testers.jl Outdated Show resolved Hide resolved
Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than missing the unhappy path tests, this seems good.
It is a breaking change, since it defaults to true, but I think that is fine?

@sethaxen
Copy link
Member Author

sethaxen commented Dec 7, 2020

It is a breaking change, since it defaults to true, but I think that is fine?

Yes, I think we do want it to be on by default. It is already exposing a fair amount of type-instability in ChainRules. It is a breaking change, and it needs a bump to minor version number.

@sethaxen
Copy link
Member Author

This is ready to merge, but because it bumps the minor version number and will require some changes to ChainRules to bring back compatibility, maybe I should wait for other non-breaking PRs to be merged first?

@willtebbutt
Copy link
Member

I have no objection to it being merged, but @oxinabox might want to get a couple of his things in first?

@oxinabox
Copy link
Member

yes, I want to get #83 and #84 in first.
and also can we leave this as -DEV as there are some deprecations to be remvoved also

@oxinabox
Copy link
Member

This can be merged now

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