-
Notifications
You must be signed in to change notification settings - Fork 587
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
Improve inference in builds() and from_type() for attrs classes #1282
Conversation
3fc0435
to
ded2110
Compare
hypothesis-python/RELEASE.rst
Outdated
RELEASE_TYPE: minor | ||
|
||
This release adds a new mechanism to infer strategies for classes | ||
defined using pypi:`attrs`, based on the the type, converter, or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:pypi:
# Try inferring type or exact values from attrs provided validators | ||
if atrib.validator is not None: | ||
validator = atrib.validator | ||
if isinstance(validator, attr.validators._OptionalValidator): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems not very legal to access third party privates. Isn't there any other ways to get those via public api?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use type(attr.validators.optional([]))
, but IMO it's actually cleaner to peek inside here. I think the alternative form is about as likely to break as the explicit form, and I strongly prefer the look of the latter.
) | ||
|
||
|
||
def from_attrs_attribute(atrib): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attrib
# Various things we know. Updated below, then inferred to a strategy | ||
base = st.nothing() # updated to none() if None is a possibility | ||
default = st.nothing() # A strategy for the default value, if any | ||
in_ = [] # list of in_ validator collections to sample from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be name it validator_collections
instead? in_
doesn't looks informative. Or there is a reason for such naming?
adc55e1
to
30a29b9
Compare
Hey @DRMacIver, can you manually cancel some of the huge stack of Appveyor builds and maybe look into auto-cancellation? (Apparently Appveyor calls this "rolling builds") |
Done and done. I thought we got your permissions on appveyor working though? |
We've looked at it a couple of times but without success; and with auto-cancellation it's NBD really. |
Hey @HypothesisWorks/hypothesis-python-contributors, this is ready for review 🎉 (and hi @hynek, if you have opinions on Hypothesis-for-Attrs I'd love to hear them too) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't seem to be quite awake enough today to do a full review of this, but I've added some comments with thoughts and some stuff that confused me.
In general I'm finding a lot of the inference code quite inscrutable. I get why it's like this - there are a lot of annoying special cases to deal with - but I think we might need better commenting at a minimum (something like the mini-essays in engine.py) and ideally some way to do a lot of this more cleanly.
default = st.nothing() # A strategy for the default value, if any | ||
in_collections = [] # list of in_ validator collections to sample from | ||
# value must be instance of all these types or tuples thereof | ||
types = defaultdict(list) # maps type to list of locations, for error msgs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got really confused about what this was doing because of the name. Maybe something like types_to_locations
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually where do we even use this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Er, I actually ended up abandoning the reporting part and this could simply be a set instead.
if attrib.validator is not None: | ||
validator = attrib.validator | ||
if isinstance(validator, attr.validators._OptionalValidator): | ||
base, validator = st.none(), validator.validator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think having these on the same line makes it less clear.
|
||
|
||
def from_attrs_attribute(attrib): | ||
"""Infer a strategy from an attr.Attribute object.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I need some more high level commenting about what's going here, separate from the code. I'm not having a very awake day, but I'm really struggling to follow the logic here.
# Try inferring from the converter, if any | ||
converter = getattr(attrib, 'converter', None) | ||
if isinstance(converter, type): | ||
# Could this ever work but give incorrect inference? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is pretty inscrutable to me.
@@ -1047,6 +1049,12 @@ def builds( | |||
value :const:`hypothesis.infer` as a keyword argument to | |||
builds, instead of a strategy for that argument to the callable. | |||
|
|||
If the callable is a class defined with :pypi:`attrs`, missing required | |||
arguments may be inferred from the type, converter, or validator (for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be better to be no-specific about this and just say "on a best-effort basis"? The details are kinda uninteresting - either it works and people barely have to know it's there, or it doesn't and people will write their own strategy here rather than tinkering with their class definitions.
This release adds a new mechanism to infer strategies for classes | ||
defined using :pypi:`attrs`, based on the the type, converter, or | ||
validator of each attribute. This inference is now built in to | ||
:func:`~hypothesis.strategies.builds` and :func:`~hypothesis.strategies.from_type`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth calling out that this will change the behaviour of strategies using builds
for attrs where there's a default type they're omitting?
e.g. if I have
@attr.s()
class Foo(object):
bar = attr.ib(default=0, converter=int)
Then previously this would have generated only Foo(0)
, but now can generate Foo(n)
for any n
.
(I think it's fine to change this, I just think it's worth explicitly mentioning)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will only change the behaviour in this case if you call builds(Foo, bar=infer)
; or succeed instead of failing if you try to build something with a required argument but no strategy.
The inference about the default value is so that inferred strategies can "shrink away" to the default value, and then None
if that would be valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right! Sorry I was forgetting how defaults and inference interacted.
Thanks for the review! If it's hard for you to follow now it'll be hard for me too later, so best to comment heavily and clarify where I can now! I'll make the docs less specific too. |
OK, that should be easier to follow now. In the process I've realised that I also need to reconcile the types in a way that supports subtypes better (i.e. at all), so that section is not likely to stay a two-liner. Otherwise pretty much done, assuming test pass 😆 |
5874a44
to
99d677f
Compare
I cannot review this code but I’m very excited by it! |
|
82ad134
to
b8a716b
Compare
Ping @DRMacIver; ready for final review and maybe even merging 😉 |
Sorry. ETA for review capacity probably not until Monday. I might be able to get some time to do review tomorrow morning but no promises. 🤞 |
(Based on what I've seen already, if someone else wants to do the review on this and is happy to sign off on it, I'm perfectly happy for them to do so! I have no problem with the feature itself - all of my questions would be implementation level. But I'm also happy to do the review on this, just temporarily time constrained) |
while i and j >= 0: | ||
result[j] = i & 255 | ||
i >>= 8 | ||
j -= 1 | ||
if i: | ||
raise OverflowError('int too big to convert') | ||
raise OverflowError('i=%r cannot be represented in %r bytes' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a transient failure here, and couldn't replicate it. Of course I haven't seen it since changing the message either, but maybe someday this will help 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth a ticket about the transient failure, pointing to this line? At least we’d have a record of the last time we (sort of) saw this failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you still have the stack trace around? Would be useful to have it in an issue for posterity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alas, no - I expected to get a better one and haven't seen it since 😭
No worries then, and no pressure! If you have a little time, I'd actually prefer a review of #1270 - it's basically "is David happy with the documentation of novel public API". @HypothesisWorks/hypothesis-python-contributors it would be nice to get this (and/or the easier #1295) merged in time for my May 31st workshop if any of you are interested in reading it 😉 |
Ping @HypothesisWorks/hypothesis-python-contributors - it would be lovely to get a review of this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments. Generally looks good. I'd be lying if I said I fully understood the inference code, but it's definitely a lot more readable now!
if strat.is_empty: | ||
raise ResolutionFailed( | ||
'Cannot infer a strategy from the default, vaildator, type, or ' | ||
'converter for %r' % (attrib,)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the repr for a an attrib object look like? Does this actually end up as a useful error message? (e.g. I'd like to see both the attribute name and the class name in this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The minimal attrib repr is Attribute(name='a', default=NOTHING, validator=None, repr=True, cmp=True, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None)
. I'll add the class repr to the message though.
return -1 | ||
return issubclass(t, collections.abc.Container) | ||
return (-1, repr(t)) | ||
if PY2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty 👎 on if PY2
checks outside of compat.py
. Couldn't this be a from hypothesis.internal.compat import Container
?
while i and j >= 0: | ||
result[j] = i & 255 | ||
i >>= 8 | ||
j -= 1 | ||
if i: | ||
raise OverflowError('int too big to convert') | ||
raise OverflowError('i=%r cannot be represented in %r bytes' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you still have the stack trace around? Would be useful to have it in an issue for posterity.
while i and j >= 0: | ||
result[j] = i & 255 | ||
i >>= 8 | ||
j -= 1 | ||
if i: | ||
raise OverflowError('int too big to convert') | ||
raise OverflowError('i=%r cannot be represented in %r bytes' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth a ticket about the transient failure, pointing to this line? At least we’d have a record of the last time we (sort of) saw this failure.
# when we try to get a value but have lost track of where this was created. | ||
if strat.is_empty: | ||
raise ResolutionFailed( | ||
'Cannot infer a strategy from the default, vaildator, type, or ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sp: “vaildator”
d8d9250
to
bf12de9
Compare
Blocked by #1348 ( |
And it's released! Long live testing things with |
Closes #954. If you have well-behaved classes defined using attrs, and use the various validation, typing, and conversion features without abusing them too badly, Hypothesis will be a little more magical ✨
The trick for users - as usual - is to handle everything that static analysis thinks might be a valid input! It's a good habit in general, and I actually kinda like the tooling pushing me that way 😄
(as an implementer, I now remember why I haven't touched inference in a while...)