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

Move float, integer, string, byte, and boolean drawing logic into PrimitiveProvider #3788

Merged
merged 45 commits into from Nov 19, 2023

Conversation

tybug
Copy link
Member

@tybug tybug commented Nov 12, 2023

Completes the first step of #3086 (comment). Thanks to @Zac-HD for helping me through the beginning of these changes IRL! (though all mistakes are mine).

There are definitely things here which I am not certain about (see review comments), but it's a start.

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.

🎉🎉🎉 Thanks so much for taking this on! I really enjoyed our conversations the other day, and am now looking forward to getting this awesome work shipped 😁

High-level comments:

  • I think the bulk of the work is already behind us, though we should expect to spend a while on the tail.
  • Most of my comments are around moving a few more pieces of code, or turning more functions into methods - or in some cases working out how to delete code.
  • I suggest ignoring the missing-coverage failures until we get everything else passing and reviewed. Aside from the detailed review comments, getting mypy passing and drafting some release notes should get us to the next stage.

onwards! 🚀

@Zac-HD Zac-HD self-assigned this Nov 13, 2023
@Zac-HD
Copy link
Member

Zac-HD commented Nov 16, 2023

Aha! We'd copied some code from OneCharStringStrategy to IntervalSet, and left the reference to len(self.intervals)... which changed that from meaning the number of elements in the set to the number of intervals in the compact representation. Swap to len(self), and it looks like we just need to make mypy and coverage happy 😁

Comment on lines +71 to +76
pp = PrimitiveProvider(d)
pp._write_float(f)

d2 = ConjectureData.for_buffer(d.buffer)
g = flt.draw_float(d2)
pp2 = PrimitiveProvider(d2)
g = pp2._draw_float()
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm accessing PrimitiveProvider directly here in anticipation of the ability to swap ConjectureData.provider for a different Provider implementation. This may not be necessary depending on how you were envisioning the backend swap occurring.

# option 1: inherit from interface, utils like _draw_float not shared between providers
class Provider(abc.ABC):
    @abc.abstractmethod
    def draw_integer(...): pass

    @abc.abstractmethod
    def draw_float(...): pass

    @abc.abstractmethod
    def draw_boolean(...): pass

    @abc.abstractmethod
    def draw_string(...): pass

    @abc.abstractmethod
    def draw_bytes(...): pass

class PrimitiveProvider(Provider): pass
class CrosshairProvider(Provider): pass


# option 2: inherit from PrimitiveProvider, utils like _draw_float available everywhere
class CrosshairProvider(PrimitiveProvider): pass

Copy link
Member

Choose a reason for hiding this comment

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

I like this separate style you've used 👍

Backend swap: we'll probably have ConjectureData consult the current backend setting (new) and look up the corresponding PrimitiveProvider it constructs in from a global AVAILABLE_BACKENDS: dict[str, str]. Then Crosshair et al can write a plugin which does AVAILABLE_BACKENDS["crosshair.hypothesis_backend.CrosshairPrimitiveProvider"] = "user-facing description here" and we're done!

There are still some direct uses of draw_bits() etc. which don't go through PrimitiveProvider around which will break that, but I'd rather ship this PR as soon as possible and then finish the cleanup in a follow-up; I think that's nicer for downstream experimentation. And of course, shrinking and the database etc. won't work until we finish the later stages of the overhaul anyway.

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.

I was so excited that I pushed some last minor fixes, and looking forward to seeing next steps once this is in 🚀

@Zac-HD Zac-HD merged commit 797606a into HypothesisWorks:master Nov 19, 2023
47 checks passed
@tybug tybug deleted the lowlevel-provider branch November 19, 2023 22:44
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.

None yet

2 participants