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

fix: replace protocol with direct subclass #2029

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Dec 22, 2022

Python typing protocols make it possible to enforce duck-typing at type-check time, i.e. ensure that an object has the appropriate methods and attributes rather than whether it inherits from another class. To partially check this at runtime there is a runtime_checkable decorator that simply test whether a given object implements the same methods (ignoring their types).

However, this apparently is much slower than I'd anticipated #2028. This issue is only a problem because we need to sanitise the user inputs to our layouts. Otherwise, once mypy is fully covered through the code-base, we could just remove our runtime input validation in most cases. Given that we want to provide helpful user errors, we need runtime checks to work. We could fast-path this with

def isinstance_(obj, type_) -> bool:
    return type.__instancecheck__(type_, obj) or type_.__instancecheck__(obj)  

but in this case, we don't truly need to duck-type backends; they're a limited abstraction that we can forgo the flexibility of protocols.

Fixes #2028

@codecov
Copy link

codecov bot commented Dec 22, 2022

Codecov Report

Merging #2029 (61f2903) into main (a242c6e) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 61f2903 differs from pull request most recent head 3e366f1. Consider uploading reports for the commit 3e366f1 to get more accurate results

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_backends.py 84.40% <100.00%> (+0.19%) ⬆️

@agoose77 agoose77 temporarily deployed to docs-preview December 22, 2022 10:29 — with GitHub Actions Inactive
@agoose77 agoose77 temporarily deployed to docs-preview December 22, 2022 10:47 — with GitHub Actions Inactive
@agoose77 agoose77 force-pushed the agoose77/fix-replace-slow-protocol branch from 61f2903 to 3e366f1 Compare December 22, 2022 11:38
@agoose77 agoose77 temporarily deployed to docs-preview December 22, 2022 11:48 — with GitHub Actions Inactive
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

I appreciate what the typing is expressing about constraints here, but as @nsmith- showed, it's adding too much time.

We talked on Zoom about this. Since we are requiring that the backend is one of (our own) four backend objects, the test could be a function in ak._util:

def is_backend(backend):
    return (
        backend is ak._backends.NumpyBackend
        or backend is ak._backends.CupyBackend
        or backend is ak._backends.JaxBackend
        or backend is ak._backends.TypeTracerBackend
    )

which is a centralized place to update if we ever add a fifth backend. That could be even faster than a standard isinstance check. (I'm describing an is check on each possibility, rather than a backend in (...) to even avoid __eq__ checks.)

But that's probably not necessary; this PR solves the immediate performance issue as-is.

@agoose77 agoose77 merged commit c15c99a into main Dec 22, 2022
@agoose77 agoose77 deleted the agoose77/fix-replace-slow-protocol branch December 22, 2022 17:57
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.

Checking isinstance of a Protocol is slow
2 participants