Migrate DataTree to the new IR

[tybug]

Another step towards #3086.

• depends on Various core and test changes #3835
• depends on Test touchups in preparation for ir migration #3844
• depends on Improvements to forced and forced testing #3846
• depends on More small IR-related improvements #3854
• follow up on investigating Various core and test changes #3835 (comment)

Potential follow-up work to this PR:

• follow draw distributions only with some meta-probability (Migrate DataTree to the new IR #3818 (comment))
• replace sub-ir examples with an example for each ir node type (this is potentially obsoleted by rebuilding the shrinker)
• don't write single-valued pseudo choice nodes to the tree (Migrate DataTree to the new IR #3818 (comment))
• don't write forced values to the tree (Migrate DataTree to the new IR #3818 (comment))
• cache compute_max_children (Migrate DataTree to the new IR #3818 (comment))

Conversations of note:

• overapproximating vs underapproximating in compute_max_children (Migrate DataTree to the new IR #3818 (comment))
• TooHard (and alternatives) Migrate DataTree to the new IR #3818 (comment)
• if observe and forced is None performance comment Migrate DataTree to the new IR #3818 (comment)
• profiling results for slowdown on this branch Migrate DataTree to the new IR #3818 (comment)

[Zac-HD]

The approach makes sense, and it looks like a good start!

Due to holiday season I'm going to unsubscribe from notifications on push, so please @-mention me to request review again when that would be useful (or at latest when CI is green) 🚀

[tybug]

@Zac-HD round two! I still have failing tests to address and things to investigate, but I've left some comments I've run into since the last update. Tests are looking a fair bit greener than last time 🙂

[tybug]

There looks to be some infinite loops (or very slow tests) here 😕. See https://github.com/HypothesisWorks/hypothesis/actions/runs/7733984648/job/21087140454?pr=3818, which I cancelled after 3.5 hours. I'll try to narrow it down locally.

[tybug]

tracked the infinite loop down and fixed it: 5058f50

[tybug]

Seems to be a remaining flake:

File "/home/runner/work/hypothesis/hypothesis/hypothesis-python/tests/cover/test_error_in_draw.py", line 28, in test_error_is_in_finally
    assert "[0, 1, -1]" in "\n".join(err.value.__notes__)
AssertionError: assert '[0, 1, -1]' in 'Falsifying example: test(\n    d=data(...),\n)\nDraw 1: [1, 0, -20]'
 +  where 'Falsifying example: test(\n    d=data(...),\n)\nDraw 1: [1, 0, -20]' = <built-in method join of str object at 0x7f6f9665fe98>(['Falsifying example: test(\n    d=data(...),\n)', 'Draw 1: [1, 0, -20]'])
 +    where <built-in method join of str object at 0x7f6f9665fe98> = '\n'.join
 +    and   ['Falsifying example: test(\n    d=data(...),\n)', 'Draw 1: [1, 0, -20]'] = ValueError().__notes__
 +      where ValueError() = <ExceptionInfo ValueError() tblen=4>.value

but I ran this locally, n ~= 10k, and it never failed. Maybe my transcription was unfaithful?

There's also this failure, which I don't know how to interpret. Is INTERNALERROR indicative of a pytest-level failure?

[Zac-HD]

(short review because it's late, forgive terseness - I thought you'd prefer this now than a more effusive review next week)

• love it and looking forward to merging real soon now. Some style nitpicks in comments.
• I think the INTERNAL ERRORs look like they're transient flakes? Suspicious that it's only on Windows, we might have gotten a bad worker or something. Hopefully vanishes on retry.
• From checking CI times this might be a slight slowdown? I think it's acceptable for now to get this in, and we can profile + optimize once we finish the big port - the design is not intrinsically any slower than the status quo (quite the opposite)

[tybug]

I appreciate the early review here ❤️. I'll never complain about productive-but-direct reviews 🙂

From checking CI times this might be a slight slowdown? I think it's acceptable for now to get this in, and we can profile + optimize once we finish the big port - the design is not intrinsically any slower than the status quo (quite the opposite)

I noticed this the other day as well. From some light profiling it seems that simulate_test_function is now almost twice as expensive in some cases, due to creating examples while simulating via draw_*. It looks like this is especially impactful in the targeting phase?

Here's some profiling results for test_targeting_increases_max_length. Apologies for cropped screenshots, no reproducing code, etc. this was all a bit ad-hoc.

master	this pr

sub-ir examples go away when we migrate the shrinker, and this doesn't seem debilitating performance wise, so I'm ok with just moving forward here if you are. We could try threading a "dont start or stop examples" var through to draw_* if we wanted a stopgap measure, but I'm only moderately certain doing so wouldn't break things elsewhere (and it would be a bit messy to get right with all the places that need it).

[Zac-HD]

Yep, let's just move forward.

I just merged the Black 2024 update so that's the formatting issue, and I think I've worked out the pytest crash too - I'll push a fix for those once the current merge finishes. And then if it's green... 🤞

[Zac-HD]

🥳🥳🥳🥳🥳🥳

[tybug]

woo!