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

Banning non runtime type in from_type #3331

Merged
merged 40 commits into from
May 11, 2022

Conversation

Cheukting
Copy link
Contributor

@Cheukting Cheukting commented May 2, 2022

Banning the non-runtime types in from_type. See #2978 and #3280 for more detail.

@Cheukting
Copy link
Contributor Author

Cheukting commented May 4, 2022

@Zac-HD I think I would like your review now, the rest of the failed tests are parallelization issues (ERROR collecting gw1) Please kindly guide me to solve the issue and any refactoring if needed.

@Zac-HD
Copy link
Member

Zac-HD commented May 6, 2022

@Zac-HD I think I would like your review now, the rest of the failed tests are parallelization issues (ERROR collecting gw1) Please kindly guide me to solve the issue and any refactoring if needed.

Ah-ha, got it: the problem is that NON_RUNTIME_TYPES is a frozenset, and on Python 3.9+ the iteration order can (and does) vary between the different pytest processes: if you look closely at the diff here, e.g. ctrl-f Never, you'll see that it's a comparison between shuffled lines. I think the best solution is to give them a stable ordering, such as:

@pytest.mark.parametrize("non_runtime_type", sorted(NON_RUNTIME_TYPES, key=lambda t: t.__name__))

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

This looks really nice Cheuk - thanks again!

hypothesis-python/RELEASE.rst Outdated Show resolved Hide resolved
@Cheukting
Copy link
Contributor Author

@Zac-HD I think I would like your review now, the rest of the failed tests are parallelization issues (ERROR collecting gw1) Please kindly guide me to solve the issue and any refactoring if needed.

Ah-ha, got it: the problem is that NON_RUNTIME_TYPES is a frozenset, and on Python 3.9+ the iteration order can (and does) vary between the different pytest processes: if you look closely at the diff here, e.g. ctrl-f Never, you'll see that it's a comparison between shuffled lines. I think the best solution is to give them a stable ordering, such as:

@pytest.mark.parametrize("non_runtime_type", sorted(NON_RUNTIME_TYPES, key=lambda t: t.__name__))

The only problem is that they don't have __name__ but if we only need a consistent order I just use str

@Cheukting
Copy link
Contributor Author

@Zac-HD pytest in 3.11 is doing something "interesting" here, the raises gives something internal again: https://github.com/HypothesisWorks/hypothesis/runs/6325869719?check_suite_focus=true#step:6:880

@Zac-HD
Copy link
Member

Zac-HD commented May 6, 2022

Hmm, I'd guess that this is a real behavior difference under Python 3.11 due to some internal change to Concatenate (this was really common up until 3.9 or so, and we are looking at internal APIs). No particular advice beyond "open a repl or debugger each in 3.10 and 3.11, and step through the logic until you see what's different".

But otherwise we're looking ready to merge!

@Cheukting
Copy link
Contributor Author

Cheukting commented May 6, 2022 via email

@Cheukting
Copy link
Contributor Author

@Zac-HD On a 2nd thought, this discrepancy is so similar to the one we found in python/cpython#92118 maybe it will get fixed in the next release

@Zac-HD
Copy link
Member

Zac-HD commented May 6, 2022

Makes sense - I'm planning to update our CI for beta1 tomorrow, and will rebase your PR on top of those changes then 👍

@Cheukting
Copy link
Contributor Author

Makes sense - I'm planning to update our CI for beta1 tomorrow, and will rebase your PR on top of those changes then 👍

Just so you know 3.11.0-beta 1 is out :-)

@Zac-HD
Copy link
Member

Zac-HD commented May 10, 2022

Yep, and pyenv updated overnight so #3337 is unblocked 😁

@Cheukting Cheukting force-pushed the ban_non_runtime_type branch from 83e939c to 87a9059 Compare May 10, 2022 21:15
Zac-HD added 7 commits May 10, 2022 20:17
Only keep the explicit names we want to refer to later
We also want to check that this works without typing_extensions installed, and on Python versions where we don't try that dependency.
Putting both lambdas on the same line allows our introspection to recover the function body, and here we are.
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Woohoo!

I ended up breaking a few things myself while refactoring, but this all looks good and ready to go 🚀

@Cheukting Cheukting requested a review from DRMacIver as a code owner May 11, 2022 05:46
@Zac-HD Zac-HD enabled auto-merge May 11, 2022 05:46
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