Use sys.monitoring for scrutineer on 3.12+ #3776

tybug · 2023-10-26T02:05:43Z

Closes #3773.

This is an RFC - please pick apart my api design decisions here! Self-review to follow.

Here's a micro-benchmark:

from hypothesis import *
from hypothesis.strategies import *
import time

N = 100

t = time.time()
for _ in range(N):
    @given(integers(), integers())
    def add(x, y):
        assert x != 0
    try:
        add()
    except Exception:
        pass

print((time.time() - t) / N)

master: 0.208
this pr: 0.166

Roughly 1.25x speed!

tybug · 2023-10-26T02:06:03Z

hypothesis-python/src/hypothesis/internal/scrutineer.py

+    MONITORING_EVENTS = {sys.monitoring.events.LINE: "trace_line"}
+
+
+def settracer(tracer: Tracer):


This is effectively formalizing Tracer as an abstraction over sys.settrace and sys.monitoring. An alternative is to make this even more formal, with an abstract class that fully implements the functionality of both, and is inherited by Tracer. This could allow hypofuzz to reuse said abstract class (Zac-HD/hypofuzz#9), depending on the kind of reuse you were thinking about.

If you think this is a good direction, I'd be open to implementing it.

See above for the context manger note - I think having two implementations we can use in the same way would be useful, but since I don't ever expect for there to be a third it seems overkill to use an abstract class.

hypothesis-python/src/hypothesis/internal/scrutineer.py

tybug · 2023-10-26T02:06:23Z

hypothesis-python/src/hypothesis/internal/scrutineer.py

+        sys.settrace(None)
+        return
+
+    sys.monitoring.free_tool_id(MONITORING_TOOL_ID)


PEP 669 claims that use_tool_id, free_tool_id, and set_events "should be regarded as slow". This PR currently calls all of these once per test execution. I haven't profiled these, but if this has an impact, we could consider registering once at the global level instead, and enabling/disabling the callbacks per test execution to avoid unwanted traces.

Hmm. I think we should just ignore this for now and do the simple thing, and we can complicate things iff profiling shows we need to later.

Zac-HD

First things first - thanks so much for taking this on! I'm really excited to pick up the speedup and any qualitative improvements that will enable down the line (e.g.: if it's fast, can we always trace?). Other quick thoughts:

having a clean context-manager abstraction seems like a good way to factor this out. Unfortunately, I think that performance considerations suggest we should not share any code, since a function call will lead to measurable slowdowns when we do it so often.
it looks like this is already super close, and would already be a serious win for users on Python 3.12, so let's aim to ship it soon!

hypothesis-python/src/hypothesis/internal/scrutineer.py

Zac-HD · 2023-10-28T14:03:47Z

hypothesis-python/src/hypothesis/internal/scrutineer.py

-                self._previous_location = current_location
+            self.trace_line(frame.f_code, frame.f_lineno)
+
+    def trace_line(self, code: types.CodeType, line_number: int):


I think that for sys.monitoring, we actually want to trace_branch - we're using lines in the existing tracer because branches aren't natively available.

However, this will also require some adjustment of the reporting and maybe analysis logic, so I suggest we get this working with line-monitoring first to get the big performance win, and then refactor to branch analysis in a follow-up.

Sounds good. I can take that on after this PR! (unless you want to adjust it yourself).

Zac-HD · 2023-10-28T14:09:45Z

hypothesis-python/src/hypothesis/internal/scrutineer.py

+    MONITORING_EVENTS = {sys.monitoring.events.LINE: "trace_line"}
+
+
+def settracer(tracer: Tracer):


See above for the context manger note - I think having two implementations we can use in the same way would be useful, but since I don't ever expect for there to be a third it seems overkill to use an abstract class.

Zac-HD · 2023-10-28T14:10:48Z

hypothesis-python/src/hypothesis/internal/scrutineer.py

+        sys.settrace(None)
+        return
+
+    sys.monitoring.free_tool_id(MONITORING_TOOL_ID)


Hmm. I think we should just ignore this for now and do the simple thing, and we can complicate things iff profiling shows we need to later.

tybug · 2023-10-28T15:36:57Z

I've rewritten as a context manager and switched to a non-reserved tool id (3) as suggested!

Zac-HD

Thanks, @tybug! (and sorry it took so long; it's been a crazy week)

I spent a while trying to work out how to use the neat DISABLE functionality before realizing that we can't actually do that until we start observing branches directly instead of lines... ah well, that's the follow-up project 🙂

A few tiny tweaks below, but this looks great to me and I'm looking forward to merging it!

hypothesis-python/src/hypothesis/internal/scrutineer.py

hypothesis-python/RELEASE.rst

Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com>

…into sys-monitoring

tybug · 2023-11-04T02:20:53Z

no need to apologize! 🙂

I'll take a look at branch coverage next week, if you don't beat me to it.

The packaging toolchain has now had python-requires support for long enough that I think it's no longer worth carrying this check and (very slightly) slowing down imports just for the sake of a different error message in a very very rare case.

Zac-HD · 2023-11-04T08:12:45Z

@tybug, I've really appreciated your recent PRs - both because they're significant technical contributions for our users, and because it's been a pleasure collaborating with you on them 😍

I've therefore extended you an offer to join the Hypothesis team. If you accept, there's no obligation to continue contributing of course (we're all volunteers!); but if you choose to review PRs you'll also have the ability to approve and merge them once you feel they're ready. Thanks again - and I hope we'll keep working together!

tybug · 2023-11-04T15:32:56Z

I'm honored! I expect nothing will change for the immediate future, as I still have nowhere near the experience required with the Hypothesis codebase to feel comfortable merging anything but the simplest PRs. But maybe my 2c will be useful on occasion.

It's been a joy working with you as well, and I intend to continue contributing to Hypothesis as I'm able to. I gladly accept 😀

use sys.monitoring for scrutineer on 3.12+

9cf85e8

tybug commented Oct 26, 2023

View reviewed changes

Zac-HD reviewed Oct 28, 2023

View reviewed changes

tybug added 3 commits October 28, 2023 11:27

change MONITORING_TOOL_ID to 3

c2a25a0

switch scrutineer to a context manager

b3864cc

add release note

1a027ca

add return type annotation

b962427

Zac-HD reviewed Nov 3, 2023

View reviewed changes

hypothesis-python/src/hypothesis/internal/scrutineer.py Outdated Show resolved Hide resolved

hypothesis-python/RELEASE.rst Outdated Show resolved Hide resolved

tybug and others added 3 commits November 3, 2023 22:07

manually inline trace_line for < 3.12 path

2ba319c

use more robust stdlib crosslinks

ab8b106

Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com>

Merge branch 'sys-monitoring' of https://github.com/tybug/hypothesis …

42b1a31

…into sys-monitoring

Zac-HD and others added 4 commits November 4, 2023 06:00

Tweak monitoring implementation

7e33ecc

Fix changelog markup

972165e

Update pinned dependencies

5438873

Remove explicit Python-version check

892bf41

The packaging toolchain has now had python-requires support for long enough that I think it's no longer worth carrying this check and (very slightly) slowing down imports just for the sake of a different error message in a very very rare case.

Zac-HD approved these changes Nov 4, 2023

View reviewed changes

Zac-HD enabled auto-merge November 4, 2023 06:15

Zac-HD added 2 commits November 4, 2023 07:10

Get CI passing

88ab8a9

fix missing cross-reference

66af9e6

Zac-HD merged commit a04236d into HypothesisWorks:master Nov 4, 2023
46 checks passed

This was referenced Nov 4, 2023

Use trace_branch monitoring for Scrutineer #3781

Open

Update pinned dependencies #3778

Closed

tybug deleted the sys-monitoring branch November 4, 2023 15:34

Zac-HD mentioned this pull request Dec 26, 2023

Add the ability to opt out of (into?) coverage reporting when collecting observability data #3821

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use sys.monitoring for scrutineer on 3.12+ #3776

Use sys.monitoring for scrutineer on 3.12+ #3776

tybug commented Oct 26, 2023 •

edited

Loading

tybug Oct 26, 2023

Zac-HD Oct 28, 2023

tybug Oct 26, 2023 •

edited

Loading

Zac-HD Oct 28, 2023

Zac-HD left a comment

Zac-HD Oct 28, 2023

tybug Oct 28, 2023

Zac-HD Oct 28, 2023

Zac-HD Oct 28, 2023

tybug commented Oct 28, 2023

Zac-HD left a comment

tybug commented Nov 4, 2023

Zac-HD commented Nov 4, 2023

tybug commented Nov 4, 2023

		MONITORING_EVENTS = {sys.monitoring.events.LINE: "trace_line"}


		def settracer(tracer: Tracer):

Use sys.monitoring for scrutineer on 3.12+ #3776

Use sys.monitoring for scrutineer on 3.12+ #3776

Conversation

tybug commented Oct 26, 2023 • edited Loading

tybug Oct 26, 2023

Choose a reason for hiding this comment

Zac-HD Oct 28, 2023

Choose a reason for hiding this comment

tybug Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Zac-HD Oct 28, 2023

Choose a reason for hiding this comment

Zac-HD left a comment

Choose a reason for hiding this comment

Zac-HD Oct 28, 2023

Choose a reason for hiding this comment

tybug Oct 28, 2023

Choose a reason for hiding this comment

Zac-HD Oct 28, 2023

Choose a reason for hiding this comment

Zac-HD Oct 28, 2023

Choose a reason for hiding this comment

tybug commented Oct 28, 2023

Zac-HD left a comment

Choose a reason for hiding this comment

tybug commented Nov 4, 2023

Zac-HD commented Nov 4, 2023

tybug commented Nov 4, 2023

tybug commented Oct 26, 2023 •

edited

Loading

tybug Oct 26, 2023 •

edited

Loading