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

Handle untyped interfaces in _check #18

Merged
merged 9 commits into from
Jan 24, 2024
Merged

Handle untyped interfaces in _check #18

merged 9 commits into from
Jan 24, 2024

Conversation

maxmynter
Copy link
Collaborator

@maxmynter maxmynter commented Jan 24, 2024

Guard match checks of types in function signature and supplied via the params kwarg in the benchmark decorators _check method against untyped function signatures.

If untyped, we do not check for matching types. We also log a warning to the stdout of the logger indicating the untyped parameter and function name.

@maxmynter maxmynter marked this pull request as ready for review January 24, 2024 10:54
@maxmynter maxmynter self-assigned this Jan 24, 2024
Copy link
Collaborator

@nicholasjng nicholasjng left a comment

Choose a reason for hiding this comment

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

With this change, the following should be a failure (non-unique types):

import nnbench

@nnbench.benchmark
def increment(value):
    return value + 1


@nnbench.benchmark
def double(value: int) -> int:
    return value * 2

Is this expected? Are we fine with this?

(We should check that the resulting type in the error message is still sensible, though. Is it <class 'empty'> by any chance?)

src/nnbench/runner.py Outdated Show resolved Hide resolved
Comment on lines 41 to 43
if not param.annotation == inspect.Parameter.empty and not issubclass(
param_types[name], param.annotation
): # only check type match if type supplied
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if not param.annotation == inspect.Parameter.empty and not issubclass(
param_types[name], param.annotation
): # only check type match if type supplied
# skip the check if benchmark interface is untyped
if param.annotation == inspect.Parameter.empty:
continue
if not issubclass(param_types[name], param.annotation):

@maxmynter
Copy link
Collaborator Author

maxmynter commented Jan 24, 2024

(We should check that the resulting type in the error message is still sensible, though. Is it <class 'empty'> by any chance?)

It is <class 'inspect._empty'>.

@nicholasjng
Copy link
Collaborator

(We should check that the resulting type in the error message is still sensible, though. Is it <class 'empty'> by any chance?)

It is <class 'inspect._empty'>.

It's fine for now. We can think about making it show up as <untyped> or something, but that requires some more effort, and would only be relevant in the case of an error, i.e. just before raising the exception.

@maxmynter
Copy link
Collaborator Author

i would argue that your example above should throw an error. Untyped and typed might very well be non-unique types. We cannot handle different types for the same variable name and if untyped cannot ensure it is the right one. So it's either on the user to keep everything untyped and ensure the right value is passed. Or the absence of the type is an oversight in which case the error is a welcome reminder.

src/nnbench/runner.py Outdated Show resolved Hide resolved
@maxmynter maxmynter merged commit d3c93f2 into main Jan 24, 2024
5 checks passed
@nicholasjng nicholasjng deleted the untyped branch January 24, 2024 18:42
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