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

Deprecate operator dispatch in Var #192

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

neNasko1
Copy link
Contributor

Operator dispatch is a too high-level feature for spox. Also it is currently clunky to use from a user standpoint as it essentially "monkey-patches" the dispatcher in spox.

A more appropriate way to access this behaviour is by using ndonnx.

Checklist

  • Added a CHANGELOG.rst entry

Copy link
Collaborator

@cbourjau cbourjau left a comment

Choose a reason for hiding this comment

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

I doesn't make a big difference, but technically we want to be sure that we warn users if they do anything that will break in the future. If we remove operator_overloading in the future any from spox._future import operator_overloading will already break regardless of usage. As such, we would ideally already warn on that level. One way to do so is to rename the function to _operator_overloading and then define a getattr function in the module to return it if a user imports "operator_overloading". We may then raise the warning from getattr rather than when the user actually calls the function. This comes with the added benefit that it would already remove the function from the documentation, too.

src/spox/_future.py Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
@neNasko1
Copy link
Contributor Author

I doesn't make a big difference, but technically we want to be sure that we warn users if they do anything that will break in the future. If we remove operator_overloading in the future any from spox._future import operator_overloading will already break regardless of usage. As such, we would ideally already warn on that level. One way to do so is to rename the function to _operator_overloading and then define a getattr function in the module to return it if a user imports "operator_overloading". We may then raise the warning from getattr rather than when the user actually calls the function. This comes with the added benefit that it would already remove the function from the documentation, too.

I think it should now work as described here^

Note: These are how the different situations get handled after the change, using the developer mode:

# warning
from spox._future import operator_overloading

# no warning
import spox._future as sf

# warning
sf.operator_overloading

# no warning
from spox._future import set_type_warning_level

# ImportError
from spox._future import non_existant_function

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.

2 participants