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

Observability - count sat/unsat calls to assume() and stateful @precondition()s #3869

Merged
merged 2 commits into from
Feb 3, 2024

Conversation

Zac-HD
Copy link
Member

@Zac-HD Zac-HD commented Feb 2, 2024

This is part of #3845, which will enable downstream tools to resolve #213 for good. Also includes some extra metadata to fix #3861, since it looked easy.

@hgoldstein95, let me know what you think - will this be useful for Tyche?

@Zac-HD Zac-HD added the legibility make errors helpful and Hypothesis grokable label Feb 2, 2024
@Zac-HD Zac-HD force-pushed the observe-predicate-outcomes branch 2 times, most recently from 51c4d21 to c55e0d2 Compare February 2, 2024 09:23
@Zac-HD Zac-HD requested a review from tybug February 2, 2024 23:31
Copy link
Member

@tybug tybug left a comment

Choose a reason for hiding this comment

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

cool stuff!

idle comment: distinguishing by line leads to overlapping reports with multiple assume statements on a single line (contrived: assume(a); assume(b), but perhaps more natural is the unidiomatic if assume(a): assume(b), or assume(a) and assume(b)). This is (1) sort of a feature that we report both here, (2) probably rare, and (3) we're only reporting this metadata and not actioning on it. All of which mean I don't think this is a problem at all.

Comment on lines 1453 to 1455
self._observability_predicates: defaultdict = defaultdict(
lambda: {"sat": 0, "unsat": 0}
)
Copy link
Member

Choose a reason for hiding this comment

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

nit: since we're exposing this in observability reports, the full names of "satisfied" and "unsatisfied" make more sense to me from the perspective of a library parsing our observability spec. Bringing this up because it's easy to change now, as opposed to when the format has stabilized a bit (and potentially has stable consumers).

@Zac-HD Zac-HD enabled auto-merge February 3, 2024 10:32
@Zac-HD Zac-HD merged commit ed6f137 into HypothesisWorks:master Feb 3, 2024
47 checks passed
@Zac-HD Zac-HD deleted the observe-predicate-outcomes branch February 3, 2024 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legibility make errors helpful and Hypothesis grokable
Projects
None yet
2 participants