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

Migrate reorder_examples to the IR #3923

Merged
merged 10 commits into from
Mar 18, 2024
Merged

Conversation

tybug
Copy link
Member

@tybug tybug commented Mar 16, 2024

Part of #3921.

There's a failure on test_optimises_the_pareto_front:

...
  File "/Users/tybug/Desktop/Liam/coding/hypothesis/hypothesis-python/tests/conjecture/test_pareto.py", line 206, in test_optimises_the_pareto_front
    assert len(runner.pareto_front) == 6
AssertionError: assert 26 == 6
 +  where 26 = len(<hypothesis.internal.conjecture.pareto.ParetoFront object at 0x13b9c68d0>)
 +    where <hypothesis.internal.conjecture.pareto.ParetoFront object at 0x13b9c68d0> = <hypothesis.internal.conjecture.engine.ConjectureRunner object at 0x13b9c6bd0>.pareto_front

I am still fuzzy on the purpose of the pareto front for cases when target() is not involved. With target(), it's clear to me that we want to keep the pareto front of all distinct target keys. But what metric are we optimizing over when there are no target calls?

But I'm even less clear why my changes broke it beyond "ParetoFront uses shrinker and this pull changes the shrinker". If I'm lucky you'll immediately have an idea what the problem is 😄 but otherwise I'll pick up here again tomorrow.

@tybug tybug requested a review from Zac-HD as a code owner March 16, 2024 05:09
@Zac-HD
Copy link
Member

Zac-HD commented Mar 16, 2024

I am still fuzzy on the purpose of the pareto front for cases when target() is not involved. With target(), it's clear to me that we want to keep the pareto front of all distinct target keys. But what metric are we optimizing over when there are no target calls?

If there are no target() calls, then we never actually run the optimizer - but... hmm. We do track the buffer in the pareto_front even if there are no target observations at all. Seems like a performance win to stop doing that, then!

But I'm even less clear why my changes broke it beyond "ParetoFront uses shrinker and this pull changes the shrinker". If I'm lucky you'll immediately have an idea what the problem is 😄 but otherwise I'll pick up here again tomorrow.

Unfortunately "something about the shrinker" is about all I've got too 😅

@tybug
Copy link
Member Author

tybug commented Mar 17, 2024

Happy to find out that the pareto test was a genuine failure! Fixed in 57d3a39; self.engine.test_function(data) was doing more than we wanted it to for ir datas, including adding it to the pareto front. I don't think this is inherently problematic, but it was conflated by some equality issues with ir datas not being equal to buffer datas despite having the same buffer, and therefore being added to the pareto front multiple times...I didn't look too deeply into why.

@tybug
Copy link
Member Author

tybug commented Mar 17, 2024

saw and fixed a flake, roughly once every 300 runs in local testing: https://github.com/HypothesisWorks/hypothesis/actions/runs/8317232496/job/22757739074#step:6:877 / d936526. Doubt it's related to these changes.

@Zac-HD
Copy link
Member

Zac-HD commented Mar 18, 2024

Just saw too_slow health check in test_data_with_misaligned_ir_tree_is_invalid on master CI, fyi.

@tybug
Copy link
Member Author

tybug commented Mar 18, 2024

noticed when I was working on this pr as well. Added a suppression for it on this branch earlier (now test_node_with_different_ir_type_is_invalid), though potentially unnecessary after we split that test.

e: nope, we're filtering too much. left a todo

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.

Looks good, merge when ready!

@tybug
Copy link
Member Author

tybug commented Mar 18, 2024

nice! rebased and added a changelog, let's hope ci is happy.

@tybug tybug merged commit 40daade into HypothesisWorks:master Mar 18, 2024
51 checks passed
@tybug tybug deleted the more-shrinker-ir branch March 18, 2024 04:19
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