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(numpy): numpy annotation processing #4795

Merged
merged 9 commits into from
Jun 14, 2024

Conversation

judahrand
Copy link
Contributor

@judahrand judahrand commented Jun 12, 2024

What does this PR address?

Currently, it seems to me that numpy annotations are broken. For example, neither of the correct types npt.NDArray[np.float32] or np.ndarray[Any, np.dtype[np.float32]], are processed and instead result in errors.

These changes fix this and add to the unit tests to catch regressions. Additionally, I've made sure that if the type hint for a Numpy array doesn't match the Annotated DType then we throw an error.

I'm really surprised how little unit testing there is in this area - am I missing something?

Fixes #(issue)

Before submitting:

@judahrand judahrand requested a review from a team as a code owner June 12, 2024 10:52
@judahrand judahrand requested review from parano and removed request for a team June 12, 2024 10:52
@judahrand
Copy link
Contributor Author

Hmmm... looks like Python 3.8 isn't happy. Will look into it.

@judahrand
Copy link
Contributor Author

Hmmm... looks like Python 3.8 isn't happy. Will look into it.

I think I got to the bottom of this. There is some weirdness in how get_origin and get_args work on Python 3.8 vs 3.11. It seems that if we try foo.__origin__ and foo.__args__ first we should get the right result.

Copy link
Contributor

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

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

LGTM.

@aarnphm aarnphm merged commit d3eef2e into bentoml:main Jun 14, 2024
37 of 39 checks passed
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.

3 participants