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

make hypothesis aware of conlist length constraints #2876

Closed
wants to merge 3 commits into from

Conversation

mdavis-xyz
Copy link
Contributor

Change Summary

This change should make hypothesis respect min_items and max_items in conlist.
But it doesn't currently, and I can't figure out why. I need help finishing this PR off.

Related issue number

#2875

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable - n/a
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

please review

@mdavis-xyz
Copy link
Contributor Author

@Zac-HD can you please help me make this PR work?
I'm trying to make hypothesis understand conlist.

@mdavis-xyz
Copy link
Contributor Author

mdavis-xyz commented Jun 3, 2021

Ah, I just saw the note up the top of _hypothesis_plugin.py.

# conlist() and conset() are unsupported for now, because the workarounds for
# Cython and Hypothesis to handle parametrized generic types are incompatible.
# Once Cython can support 'normal' generics we'll revisit this.

What's the status of those Cython changes?

And if I can't modify pydantic to make hypothesis understand conlist, is there an easy workaround to add to my end-user code to make it work? (Including creating my own class instead of conlist.) I couldn't get register_type_strategy to work. It kept throwing errors about registering a function instead, but when I did that the strategy wasn't invoked. Is this the same issue described in that comment above?

@Zac-HD
Copy link
Contributor

Zac-HD commented Jun 3, 2021

I'm sorry, but I can't help make this work. Believe me, if I could it would already be done!

Based on cython/cython#3537 and cython/cython#4005, it looks like the relevant changes to Cython have been made, but not yet released. I haven't actually tried building anything with Cython master (or 3.0a7) though, so there's a nontrivial chance that it's still broken.

It does sound like the same issue - if you have time (unfortunately I don't at the moment), you could try this patch and/or register_type_strategy under the Cython alpha. If you can share the register_type_strategy code I might be able to diagnose that, too.

Re: workarounds, registering a strategy for the model you want using st.builds() should work - and pass a st.lists() strategy for the conlist argument.

@mdavis-xyz
Copy link
Contributor Author

Those links are for cython, but I'm using cpython. My understanding is that they are competing interpreters, so any user is going to use just one or the other. So why does it still not work for me in cpython?

For the workaround, you're saying to register the class whose attribute is a conlist?

class Model(BaseModel):
    x: conlist(int, min_items=1)
register_type_strategy(Model, something)

Oh boy. My real model's going to have about 20 or 30 different conlist attributes. I'll see if I can automate that.

@Zac-HD
Copy link
Contributor

Zac-HD commented Jun 4, 2021

Those links are for cython, but I'm using cpython. My understanding is that they are competing interpreters, so any user is going to use just one or the other. So why does it still not work for me in cpython?

Cython is not an interpreter, it's used to compile libraries such as Pydantic as native-code extensions for CPython. See this code in setup.py.

Oh boy. My real model's going to have about 20 or 30 different conlist attributes. I'll see if I can automate that.

Shouldn't be too hard, since you have access to the Model object and can call typing.get_type_hints() on it 🙂

@mdavis-xyz
Copy link
Contributor Author

Ah I see.
So if I'm using pydantic, then I'm using cython, even if I'm not aware of it.

Once those other issues are resolved, is the code in this PR sufficient to to resolve #2876 ?

@mdavis-xyz
Copy link
Contributor Author

mdavis-xyz commented Jun 5, 2021

Here's my workaround script.

Zac, can you please tell me the best way to do the if statement condition? I couldn't find anything better than if 'ConstrainedListValue' in str(t):, since the types library doesn't have a ConstrainedListValue class that can be imported.

import pydantic
from pydantic import conlist
from typing import Type, List, Optional, get_type_hints
from hypothesis import strategies as st
from hypothesis import given

class Problem(pydantic.BaseModel):
    x: int
    y: List[str]
    z: conlist(str, min_items=4, max_items=5)
    a: conlist(conlist(float, min_items=1, max_items=2), max_items=1)

def strat_from_type(t: Type) -> st.SearchStrategy:
    #if isinstance(t, types.ConstrainedListValue): # doesn't work this way
    if 'ConstrainedListValue' in str(t):
        return st.lists(strat_from_type(t.item_type), min_size=t.min_items or 0, max_size=t.max_items)
    else:
        return st.from_type(t)

def build(**kwargs):
    return kwargs

strat = st.builds(build, **{k: strat_from_type(t) for (k,t) in get_type_hints(Problem).items()})

@given(strat)
def test(data):
    Problem.parse_obj(data)
test()

@Zac-HD
Copy link
Contributor

Zac-HD commented Jun 5, 2021

Shorter workaround - note use of issubclass, omitting arguments instead of from_type(), etc.

import pydantic
from pydantic import conlist
from typing import List, get_type_hints
from hypothesis import given, strategies as st

class Problem(pydantic.BaseModel):
    x: int
    y: List[str]
    z: conlist(str, min_items=4, max_items=5)
    a: conlist(conlist(float, min_items=1, max_items=2), max_items=1)

kwargs = {
	k: st.lists(strat_from_type(t.item_type), min_size=t.min_items or 0, max_size=t.max_items)
	for k, t in get_type_hints(Problem).items()
	if issubclass(t, ConstrainedListValue)  # let builds() handle the others
}
strat = st.builds(Problem, **kwargs)

st.register_type_strategy(Problem, strat)  # you can work out other integrations or helper functions

As a more general tip, it looks like you should check out st.fixed_dictionaries():

- def build(**kwargs):
-     return kwargs
- strat = st.builds(build, **{ ... })
+ strat = st.fixed_dictionaries({ ... })

@mdavis-xyz
Copy link
Contributor Author

I already tried something similar.
That approach doesn't work.

Traceback (most recent call last):
  File "test.py", line 12, in <module>
    kwargs = {
  File "test.py", line 15, in <dictcomp>
    if issubclass(t, ConstrainedListValue)  # let builds() handle the others
NameError: name 'ConstrainedListValue' is not defined

ConstrainedListValue is a special class. It seems to not be importable from any library.

I think it comes from here.

return new_class('ConstrainedListValue', (ConstrainedList,), {}, lambda ns: ns.update(namespace))

Where new_class is from typing.

How do I make issubclass work with new_class?

@Zac-HD
Copy link
Contributor

Zac-HD commented Jun 8, 2021

Ah, sorry - we'll need to check issubclass(t, ConstrainedList) - that's the "grandparent" type, ConstrainedListValue is the parent type but not directly accessible.

@PrettyWood
Copy link
Member

Hi @mdavis-xyz
What is the status of this PR? Do you need some help on this?
IMO it can wait for after 1.9 (I try to review everything to make a release this month). Do you agree?

@mdavis-xyz
Copy link
Contributor Author

The status is that the PR doesn't work because of a bug/limitation with cython. We need to wait until Cython releases a version with those patches.

@samuelcolvin
Copy link
Member

If this is now working, we can review it and consider merging, if it's still blocked by cython, let's close it and wait for V2 when this stuff should be fixed, partly via annotated-types.

@samuelcolvin
Copy link
Member

Still failing :-(.

Let's wait to fix the problem in V2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants