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

Implement extend for cached_test_function_ir #4159

Merged
merged 8 commits into from
Nov 9, 2024

Conversation

tybug
Copy link
Member

@tybug tybug commented Nov 8, 2024

Implement extend for cached_test_function_ir, and use ir serialization for a notion of size.

previous description

Closes . #3864.

This is really two PRs:

  • implement extend: int = 0 for cached_test_function_ir
  • migrate explain to the ir

In implementing extend for the ir, we have to choose a notion of "size" for the ir. I've chosen len(nodes) for now. We'll probably want to use something more intelligent in the future, such that 1k booleans is smaller than 1k strings each with 10 characters.

Inquisitor migration was a relatively straightforward lifting of blocks to nodes. I haven't thought too carefully about whether the transformation was correct (for some explain-phase-specific-reason) outside of "tests pass", so that may be worth a think on review.

Likely easiest to review by commit (except there is an "oops, all fixes" grab bag commit at the end).

@Zac-HD
Copy link
Member

Zac-HD commented Nov 8, 2024

  • I'm not convinced the notion of ir_size makes sense; we seem to be conflating shrink ordering with number of nodes - I'd suggest separating these, and just calling len() directly for the latter.
  • I'm concerned that replacing start:end with random values will be rather inefficient for the explain mode; can we measure this or maybe implement an "if you see this magic value (None?), generate randomly until the corresponding .stop_example() call" feature.

otherwise looks awesome! Might be easiest to split out the explain mode changes to a separate PR; we could probably merge the rest today if so.

@tybug
Copy link
Member Author

tybug commented Nov 8, 2024

  • Agreed these are two distinct problems. I guess what I was trying to get at is that - in the future where BUFFER_SIZE_IR replaces BUFFER_SIZE - we are now limiting the maximum size of examples to n nodes instead of n bytes. But a single node can be almost arbitrarily large, whereas a byte can't. So if we interpret "ir size" as the number of nodes, we would allow consumers to generate e.g. a million characters in a single string without overrunning. That was the motivation behind defining a separate notion of size for overruns (which would be distinct from shrink ordering).

  • Hmm, is the new code not equivalent to the old? I didn't read the explain algorithm in detail, but it seemed to already be replacing start:end with a random buffer:

                buf_attempt_fixed = bytearray(buffer)
                buf_attempt_fixed[start:end] = [
                    self.random.randint(0, 255) for _ in range(end - start)
                ]
                result = self.engine.cached_test_function(
                    buf_attempt_fixed, extend=BUFFER_SIZE - len(buf_attempt_fixed)

and now we're replacing start:end with the same number-and-type-and-kwargs of random nodes. end - start is smaller on the ir though because that's # nodes, not # bytes.

@Zac-HD
Copy link
Member

Zac-HD commented Nov 8, 2024

Right, yeah, I think "n bytes when serialized" might be the easiest way to get a nice notion of IR size for overruns.

It was, yep. Even with type-matching though it's much less likely that we can successfully 'slip' between different valid node types if there's some variation there... maybe we just don't worry about that for now though, and leave better-generation on the todo list.

@tybug
Copy link
Member Author

tybug commented Nov 8, 2024

ah yup, serialized size is probably good enough for now (and maybe ever).

I think that after #4086 the 'slipping' is roughly fine - we don't throw away misalignments anymore. It's not as efficient in clock cycles (due to ir -> buffer -> ir), but I think we accept the hit and just try to move off bytes asap.

@tybug
Copy link
Member Author

tybug commented Nov 9, 2024

OK, I've scoped down this PR. I think we're reaching a critical juncture here where it's important to get the ir semantics right, but there are spurious potential problems caused by the interaction of the ir and buffer semantics - such as disagreements on when something is an overrun. Hopefully we (I) can blaze through the changes and minimize impact.

(to be clear, I think I've avoided any consumer-facing problems, but it does make me nervous.)

@tybug tybug changed the title Migrate explain phase to the typed choice sequence Implement extend for cached_test_function_ir Nov 9, 2024
@@ -671,3 +673,75 @@ def move(self, src: bytes, dest: bytes, value: bytes) -> None:

def delete(self, key: bytes, value: bytes) -> None:
raise RuntimeError(self._read_only_message)


def ir_to_bytes(ir: Iterable[IRType], /) -> bytes:
Copy link
Member Author

@tybug tybug Nov 9, 2024

Choose a reason for hiding this comment

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

This implementation is lifted from your branch, with two fixes:

  • surrogatepass instead of surrogateescape (I don't recall my at-the-time justification of this, but if you run your test case for long enough you will get an error with surrogateescape)
  • correct interpretation for negative ints? I think this also is caught by the test case if run for long enough

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 - onwards!

@tybug tybug merged commit 80942bc into HypothesisWorks:master Nov 9, 2024
48 checks passed
@tybug tybug deleted the explain-ir branch November 9, 2024 16:00
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