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

perf: add a flag to turn off isinstance in TypeTracerArray._new #3054

Merged

Conversation

jpivarski
Copy link
Member

The runtime type-checks in TypeTracerArray._new can now be turned off by setting

awkward.typetracer.TypeTracerArray.runtime_typechecks = False

which is a developer-public (but user-hidden) global interface.

@jpivarski
Copy link
Member Author

I forgot to say, cc: @martindurant and @lgray.

@martindurant
Copy link
Contributor

Please ping me when it's in. In dak, I can make the context manager right away, and it will have no downside: before this change it will simply have no effect.

@jpivarski jpivarski merged commit 1bf2c9f into main Mar 21, 2024
38 checks passed
@jpivarski jpivarski deleted the jpivarski/flag-to-turn-off-isinstance-in-TypeTracerArray._new branch March 21, 2024 23:09
@jpivarski
Copy link
Member Author

Does this really need to be a context manager in dask-awkward, @martindurant? Because it is now a public (though hidden) interface and Coffea can just

awkward.typetracer.TypeTracerArray.runtime_typechecks = False

Being a context manager suggests that one would want to turn it off only for limited times, but I think Coffea wants to turn it off globally and only revisit that if there are any errors, right, @lgray?

@martindurant
Copy link
Contributor

With this, you have both possibilities. I would say that, generally, a library shouldn't change any state in another library except in limited scopes.

@jpivarski
Copy link
Member Author

Oh, that's right—Coffea might not be the only user of Awkward in a Python process. I was thinking of this as a global, set-it-once thing, like vector.register_awkward(), but that only adds behaviors to ak.behavior (which, in principle, might overshadow another library's, but in practice likely doesn't). This setting doesn't append state, it replaces state, and I can see how that would go wrong.

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.

awkward._nplikes.typetracer.TypeTracerArray._new needs further optimization, it is hot code
2 participants