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 recursion issues #3687

Merged
merged 14 commits into from Jul 10, 2023
Merged

Fix recursion issues #3687

merged 14 commits into from Jul 10, 2023

Conversation

jobh
Copy link
Contributor

@jobh jobh commented Jun 28, 2023

Trying to finally kill that recursion zombie.

from_type didn't properly detect recursion on types that went through types.from_typing_type. Fixed by moving the recurse-guard list to a ContextVar. (yep, I should have taken your advice on that in the first place @Zac-HD)

Additionally:

  • when nicerepr encounters an error (RecursionError for example), convert it to a warning and return a string anyway. This is what made it easy to identify the problem in the first place,
  • hard-code the stack depth increment as 2000 to avoid inconsistency between test runners (pytest seems to bump it from 1000 to 3000 during test execution, for example, and other packages may interfere),
  • some other minor stuff which I'll comment in the code.

Closes: #3525

@@ -958,14 +959,13 @@ def builds(
from hypothesis.strategies._internal.types import _global_type_lookup

for kw, t in infer_for.items():
if (
getattr(t, "__module__", None) in ("builtins", "typing")
Copy link
Contributor Author

@jobh jobh Jun 28, 2023

Choose a reason for hiding this comment

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

The example in #3026 (comment), actually didn't work now, because Optional[RecursiveClass].__module__ is typing. (RecursiveClass|None).__module__ is types, so that happened to work. And builtins is probably also unnecessary because relevant builtins are registered in _global_type_lookup anyway.

Long story short: Unless there's a reason I don't see, just remove the __module__ check and everything is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just FYI, this was the cause of the ghostwriter failures (now fixed).

@jobh

This comment was marked as outdated.

hypothesis-python/src/hypothesis/internal/reflection.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/core.py Outdated Show resolved Hide resolved
hypothesis-python/RELEASE.rst Outdated Show resolved Hide resolved
@Zac-HD

This comment was marked as outdated.

@jobh
Copy link
Contributor Author

jobh commented Jul 6, 2023

@Zac-HD I rebased this PR without the fluff, but including some cherry-picks from #3688.

@jobh jobh requested a review from Zac-HD July 6, 2023 12:39
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.

I'm looking forward to getting this in - see comments below, add release notes, and it looks like we'll need to do something about os.environ in the ghostwriter too 🤔

@jobh
Copy link
Contributor Author

jobh commented Jul 6, 2023

I'm looking forward to getting this in - see comments below, add release notes, and it looks like we'll need to do something about os.environ in the ghostwriter too thinking

I think I addressed everything (except ghostwriter), let's see how the tests turn out. Will revisit if not, but it may be a few days again

@jobh
Copy link
Contributor Author

jobh commented Jul 6, 2023

Synced and rebased to fix conflict

@Zac-HD
Copy link
Member

Zac-HD commented Jul 7, 2023

Let me know if you want me to dig in and offer some suggestions for or to push a fix for the ghostwriter issue; by default I'm happy to leave it to your judgement.

@jobh
Copy link
Contributor Author

jobh commented Jul 10, 2023

Let me know if you want me to dig in and offer some suggestions for or to push a fix for the ghostwriter issue; by default I'm happy to leave it to your judgement.

I committed a change to ghostwriter to just undo the earlier (new) deferring. I'm not familiar enough with it to tell whether this is sufficiently precise or robust

691b021

)


def _from_type(thing: Type[Ex], recurse_guard: List[Type[Ex]]) -> SearchStrategy[Ex]:
_recurse_guard: ContextVar = ContextVar("recurse_guard", default=[])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, this will actually share the same list across threads (contexts), won't it. I'll push a fix but it will add a bit of ugly

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.

Ah, the merge conflict is from this ongoing merge and should resolve itself momentarily.

Content looks great though, so I'll merge as soon as that previous release finishes!

@Zac-HD Zac-HD closed this Jul 10, 2023
@Zac-HD Zac-HD reopened this Jul 10, 2023
@Zac-HD Zac-HD enabled auto-merge July 10, 2023 18:14
@Zac-HD Zac-HD merged commit 12a52e1 into HypothesisWorks:master Jul 10, 2023
47 checks passed
@jobh jobh deleted the recursion-fixes branch July 11, 2023 11:28
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.

RecursionError with recursive pydantic model and a default value
2 participants