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

Catch BackendNotImplementedError raised from __ua_function__ #199

Merged

Conversation

@peterbell10
Copy link
Collaborator

peterbell10 commented Sep 16, 2019

Closes #197

This catches any BackendNotImplementedErrors raised by __ua_function__ and stores them in a list until the end of the function call process. At which point if no backends have been called, the exception objects are made into a tuple before raising the final exception object.

This gives errors such as

BackendNotImplementedError: ('No selected backends had an implementation for this function.', BackendNotImplementedError('Foo'))

Questions:

  • Should the nested-exceptions be associated with their backend? e.g. the tuple could contain tuples of (backend, exception).
  • Should the exception list include backends that returned NotImplemeted?
@hameerabbasi

This comment has been minimized.

Copy link
Contributor

hameerabbasi commented Sep 16, 2019

Should the exception list include backends that returned NotImplemeted?

I think not, those failed gracefully.

Should the nested-exceptions be associated with their backend? e.g. the tuple could contain tuples of (backend, exception).

That could be useful.

@hameerabbasi

This comment has been minimized.

Copy link
Contributor

hameerabbasi commented Sep 16, 2019

Do you have benchmarks for how much slower this is in the nominal case?

@peterbell10

This comment has been minimized.

Copy link
Collaborator Author

peterbell10 commented Sep 16, 2019

In theory, if BackendNotImplemented isn't raised anywhere then the speed difference should be within measurement error as it's essentially just one extra (non-taken) branch per loop.

Just running scipy.fft with this PR:

In [23]: %timeit scipy.fft.fft(x)
4.43 µs ± 171 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

vs. uarray master:

In [15]: %timeit scipy.fft.fft(x)
4.31 µs ± 194 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

Both are within 1 sigma so hard to say if it's made a difference.

If I add a backend that unconditionally fails then things definitely slow down but it's hard to say how much of that is from returning vs raising.

class Backend:
     __ua_domain__ = 'numpy.scipy.fft'
     @staticmethod
     def __ua_function__(f, a, kw):
         return NotImplemented # on master
         raise ua.BackendNotImplementedError # on this PR
Branch 1 failure 2 failures 3 failures
PR 4.86 5.42 5.96
Master 4.57 4.88 5.13
Copy link
Contributor

hameerabbasi left a comment

One minor comment. Other than that this looks all good to me.

@@ -16,20 +16,23 @@ class Backend:
__ua_domain__ = "ua_tests"


def create_nullary_mm(default=None):

This comment has been minimized.

Copy link
@hameerabbasi

hameerabbasi Sep 16, 2019

Contributor

Can you convert this to a PyTest fixture? That’s the standard way to handle things like these,

This comment has been minimized.

Copy link
@peterbell10

peterbell10 Sep 16, 2019

Author Collaborator

Done

@hameerabbasi

This comment has been minimized.

Copy link
Contributor

hameerabbasi commented Sep 17, 2019

Feel free to merge whenever you feel this is ready.

Copy link
Contributor

hameerabbasi left a comment

LGTM! Thanks, @peterbell10!

@hameerabbasi hameerabbasi merged commit 40296c6 into Quansight-Labs:master Sep 17, 2019
1 check passed
1 check passed
Quansight-Labs.uarray #20190916.3 succeeded
Details
@peterbell10 peterbell10 deleted the peterbell10:catch-be-not-implemented branch Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.