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 and improve hypothesis.extra.numpy typing #3889

Conversation

JonathanPlasse
Copy link
Contributor

@JonathanPlasse JonathanPlasse commented Feb 18, 2024

Hi,
Love hypothesis.
I have been using it for years now.
Some times ago, I stumbled on a problem when using Pyright in strict mode.
When importing hypothesis.extra.numpy.arrays, Pyright reported the error reportUnknownVariableType.
I opened this issue microsoft/pyright#7279 and came here to fix it.
Before:
Screenshot from 2024-02-18 14-55-11
After:
Screenshot from 2024-02-18 14-54-13

There are more to address like array_dtypes, from_dtype, scalar_dtypes that I could address in future PRs.

Also, I am interested in starting to contribute to Hypothesis.
To get started, I was thinking of adding type hints to the entirety of hypothesis.extra.numpy and the internal it calls.
Would this be welcomed?

@JonathanPlasse JonathanPlasse force-pushed the fix-extra-numpy-arrays-type-signature branch 2 times, most recently from a8139c9 to 9b19532 Compare February 18, 2024 13:49
@JonathanPlasse
Copy link
Contributor Author

MyPy does not complain locally of the error in the CI.

@Zac-HD
Copy link
Member

Zac-HD commented Feb 18, 2024

Wow - thanks so much @JonathanPlasse, this looks fantastic!

  • To ensure that we don't accidentally break this in future, it'd be great to add some tests to test_pyright.py modelled on this test for mypy
  • I think mypy's complaint looks like a bug, but we don't want to # type: ignore it because of pyright... maybe reversing the order of overloads will resolve it? Hopefully pyright still picks the more-specific overload...

I would be delighted to accept PRs for better type annotations across our public API. For internals, we're happy to annotate where it's simple, but correct types would often be very very complex and at some point it's not worth the overhead. Keep it simple and we're usually happy though!

@Zac-HD
Copy link
Member

Zac-HD commented Feb 27, 2024

@JonathanPlasse let me know if there's anything I can help with; there's no rush but also no need to stay stuck if you'd prefer to hand over part of the todo list 🙂

@JonathanPlasse
Copy link
Contributor Author

@Zac-HD I am currently in holidays.
I will have more time to work on this next week.
The mypy issue seems weird to me as I cannot reproduce it locally with the same version of MyPy and config.
I think there could be a bug in the CI test.
If you find a way to make it work, it will take it gladly and move on to the other typing issues.

@Zac-HD Zac-HD force-pushed the fix-extra-numpy-arrays-type-signature branch from 8f91a65 to 51a37e3 Compare March 3, 2024 02:46
@JonathanPlasse JonathanPlasse changed the title Fix hypothesis.extra.numpy.arrays type signature Fix and improve hypothesis.extra.numpy typing Mar 3, 2024
@Zac-HD Zac-HD mentioned this pull request Mar 3, 2024
@Zac-HD
Copy link
Member

Zac-HD commented Mar 3, 2024

OK, remaining items before I can merge this - we're close!

  • ensure that tests pass (currently running); may require getting pyright to resolve the numpy import
  • tests for the more precise dtypes strategies to confirm that we better resolve the elements types. Include direct check for each, and at least one that it flows through to arrays() elements.

I'm really excited about the new precision about elements types, that's going to be great for users 😁

@Zac-HD
Copy link
Member

Zac-HD commented Mar 4, 2024

Ah, got it! We needed to (1) install numpy into the test environment, and (2) point pyright to the right Python interpreter.

I've checked locally that the dtypes strategies git picked up properly; since I'm short on time I'm going to merge the PR now (it's clearly an improvement on the status quo!) and we can come back and get some additional tests into CI later.

Thanks again @JonathanPlasse 😍

@Zac-HD Zac-HD merged commit b9f3d75 into HypothesisWorks:master Mar 4, 2024
48 checks passed
@JonathanPlasse JonathanPlasse deleted the fix-extra-numpy-arrays-type-signature branch March 4, 2024 07:23
@JonathanPlasse
Copy link
Contributor Author

You are welcome!
It was great collaborating with you.
You have been a great help.

Will come back for more.

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.

None yet

2 participants