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

Use ConjectureData.for_choices #4219

Merged
merged 7 commits into from
Jan 7, 2025
Merged

Conversation

tybug
Copy link
Member

@tybug tybug commented Dec 29, 2024

Depends on #4217 (so test_draw_to_overrun finishes in a sane amount of time).

Here are three proposals for a replacement for ConjectureData.for_buffer on the typed choice sequence:

(1) pass the value of each choice:

    @classmethod
    def for_choices(
        cls,
        choices: Sequence[NodeTemplate | IRType],
        ...
    ):
        ...

(2) pass the value of each choice and its complexity index:

    @classmethod
    def for_choices(
        cls,
        choices: Sequence[NodeTemplate | tuple[IRType, int]],
        ...
    ):
        ...

(3) pass the value of each choice and its kwargs, noting that the complexity of a choice can be derived from its value + kwargs:

    @classmethod
    def for_nodes(
        cls,
        nodes: Sequence[NodeTemplate | IRNode],
        ...
    ):
        ...

I am proposing we use (1) in this pull. The benefits of this is mainly a simplified and unified api. It also makes constructing a ConjectureData from a database entry possible, where we do not have choice complexity available. However, I am leaning towards encoding complexity alongside db values anyway, for more precise sorting, so this may be a nonissue.

The downside of (1) is that we cannot use choice complexity during drawing, in particular misalignment. This leads to 129da4b, which fills misalignments with the zero-index choice instead of using complexity as an intermediary. I think using NodeTemplate to opt-in to controlling this misalignment behavior in the shrinker could be a more clean path forward. In the mean time, the default and only behavior for correcting misalignments would be filling with the zero-index choice.

The shrinking benchmark is neutral:
shrinking

@DRMacIver if you have any thoughts on this from a shrinking perspective I'd definitely be curious to hear them.

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.

LGTM overall, feel free to merge once the comments below are handled to your satisfaction.

return value

value = node.value
return value # type: ignore # mypy misses eliminating the NodeTemplate type
Copy link
Member

Choose a reason for hiding this comment

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

I'd be inclined to assert isinstance(...) rather than ignoring, since this is unlikely to be a noticable perf hit.

Copy link
Member Author

Choose a reason for hiding this comment

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

assert isinstance(ChoiceT) is impossible and assert not isinstance(NodeTemplate) feels bad - I went for a slightly more roundabout type checking solution.

@tybug tybug requested a review from DRMacIver as a code owner January 6, 2025 21:49
@tybug tybug merged commit f24f108 into HypothesisWorks:master Jan 7, 2025
47 checks passed
@tybug tybug deleted the for-choices branch January 7, 2025 00:40
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