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

Extend tests to include newly-added data types #759

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

rmitsch
Copy link
Contributor

@rmitsch rmitsch commented Sep 9, 2022

Description

Extend tests to include data types added via relaxed signatures in #599.

Types of change

Enhancement / chore.

Checklist

  • I confirm that I have the right to submit this contribution under the project's MIT license.
  • I ran the tests, and all new and existing tests passed.
  • My changes don't require a change to the documentation, or if they do, I've added all required information.

In response to #599 (comment).

@rmitsch rmitsch added enhancement Feature requests and improvements tests Tests and test suite feat / types Type hints and type checking labels Sep 9, 2022
@rmitsch rmitsch self-assigned this Sep 9, 2022
@rmitsch
Copy link
Contributor Author

rmitsch commented Sep 9, 2022

@danieldk Does this kind of modification to the tests reflect what you had in mind? If so, I'll go on adjusting the others that are relevant w.r.t. #599 (comment).

@danieldk
Copy link
Contributor

danieldk commented Sep 9, 2022

@danieldk Does this kind of modification to the tests reflect what you had in mind? If so, I'll go on adjusting the others that are relevant w.r.t. #599 (comment).

Looks good indeed!

I don't think this is easily possible, but maybe something we want to look into longer term -- ideally, we'd generate examples based on the type signatures. So, if the type signature would change, the generated data would change with it.

Though, I think that's probably a very large undertaking, so going though the types and trying to modify the tests accordingly seems like the best short-term solution.

@rmitsch
Copy link
Contributor Author

rmitsch commented Sep 9, 2022

I don't think this is easily possible, but maybe something we want to look into longer term -- ideally, we'd generate examples based on the type signatures.

That's interesting - is there already a concept on how to do that? I can imagine having a function deriving input types from another function like

@pytest.mark.parametrize("cpu_ops", CPU_OPS)
@settings(max_examples=MAX_EXAMPLES * 2, deadline=None)
@given(X=strategies.generate_examples_for(ops.flatten))
def test_flatten_unflatten_roundtrip(cpu_ops: NumpyOps, X: numpy.ndarray):
    ...

where strategies.generate_examples_for() uses inspect.signature(ops.flatten) to fetch the signature and then translates Thinc types to numpy types using _Array.dtype(), and finally picks a hypothesis strategy based on the numpy type.

This sketch doesn't feel like that much work though, so I'm probably overlooking something 😄

@danieldk
Copy link
Contributor

That's interesting - is there already a concept on how to do that?

No, I don't think we have anything like that yet.

This sketch doesn't feel like a that much work though, so I'm probably overlooking something 😄

Try it 👍. As long as it's opt-in per test, we could enable them gradually. For reviewing I think it's the nicest to do this as a proof of concept for maybe two or three methods first. Avoids a lot of rewrites as a result of review comments. Then once we are happy with the design, we could apply it to more methods.

@rmitsch
Copy link
Contributor Author

rmitsch commented Sep 15, 2022

Looked into this a bit. From Python 3.7 upwards this kind of type resolution would definitely work, 3.6 and below is a bit of a pain. To resolve function return type annotations into proper types, something like this would be necessary in 3.6 (fn is a Callable[..., Any] for a Callable returning e.g. an Union:

import inspect
from typing import _eval_type
# Extract all types in Union. This only works if those types are imported already.
types = [_eval_type(arg, globals(), globals()) for arg in inspect.signature(fn).return_annotation.__args__]

Whereas in Python 3.7+ it'd be

from typing import get_type_hints, get_args  # type_extensions in 3.7, typing in 3.8+
types = get_args(get_type_hints(fn)["return"]])

So in 3.6 we'd have to use private method and resolve the (_)ForwardReference values in inspect.signature(fn).return_annotation.__args__ to actual types, and we'd have to import all types before. Assuming there isn't any other, more elegant way that eluded me.
3.7+ returns them already in a usable format and requires only the public API.

Considering this I'd recommend implementing the type-dynamic testing once thinc drops support for 3.6. What do you think?
Meaning I'd adjust the tests as suggested in the current draft and add a follow-up ticket.

@danieldk
Copy link
Contributor

Considering this I'd recommend implementing the type-dynamic testing once thinc drops support for 3.6. What do you think?

Sounds completely sensible to me, we should avoid relying on internals of external dependencies.

@richardpaulhudson
Copy link
Contributor

An alternative to waiting for 3.6 support to be dropped before implementing the tests would be to decorate them with

@pytest.mark.skipif(sys.version_info < (3, 7), reason="test tooling requires python3.7 or higher")

@netlify
Copy link

netlify bot commented Jun 27, 2023

👷 Deploy request for thinc-ai pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 62d1a83

@svlandeg svlandeg changed the base branch from master to main April 19, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements feat / types Type hints and type checking tests Tests and test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants