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

Check for inferrability #1

Open
pxl-th opened this issue Jan 17, 2022 · 5 comments
Open

Check for inferrability #1

pxl-th opened this issue Jan 17, 2022 · 5 comments

Comments

@pxl-th
Copy link

pxl-th commented Jan 17, 2022

I'd argue that we also need to check for inferrability of a function.
Since it is an important factor when considering performance, see for example: FluxML/NNlib.jl#370.

@dfdx
Copy link
Owner

dfdx commented Jan 17, 2022

I'm thinking of adding type inference checks as a separate column (e.g. using JET.jl as suggested) - test_rrule(..., check_inference=true) fails the whole test as if the rrule() itself were broken for the specified types, which is not the goal of this check. Do you think it will be a sufficient solution?

@pxl-th
Copy link
Author

pxl-th commented Jan 18, 2022

I guess it fails, because Test.@inferred errors on the first non-inferrable function and ChainRulesTestUtils doesn't do any special handling in this case.
test_rrule() probably needs to wrap @inferred in a separate @test, like here, to avoid hard failure.

But using JET.jl is a good idea, although I haven't used it yet.

@dfdx
Copy link
Owner

dfdx commented Jan 19, 2022

I think the point of test_rrule() is to make all possible checks unless it's instructed otherwise. This is really useful when you develop a new rrule, but causes unnecessary noise in massive checks like in HQDL.

I added JET check to the report and it caught a few cases. I didn't look at them closely yet, but it's in my todo list :)

@sethaxen
Copy link

I think the point of test_rrule() is to make all possible checks unless it's instructed otherwise. This is really useful when you develop a new rrule, but causes unnecessary noise in massive checks like in HQDL.

Would it be sufficient if each type of check in test_rrule was in a different test set? Then perhaps one could parse the result to report which test set had failed. e.g. rrule inferrible would be one, pullback inferrible another, etc.

@dfdx
Copy link
Owner

dfdx commented Jan 24, 2022

test_rrule doesn't do any magic, we can write any checks ourselves. In fact, in some tests I already check primals and pullbacks, as well as correctness and speed separately.

As a side note, even current simple tests uncovered a lot of subtleties and inconsistencies. I'm trying to balance between adding more tests and closing the major gaps, so fine-grained checks for inferrability will not arrive immediately. But later or sooner, they will definitely do!

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

No branches or pull requests

3 participants