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

Fix signature codec with tuple type annotations #1064

Merged
merged 2 commits into from
Apr 4, 2023

Conversation

sauerburger
Copy link
Contributor

@sauerburger sauerburger commented Mar 29, 2023

Fixes #1036

I'm unsure how/where to add a test for this as I didn't manage to run the test suite. Locally, I've experimented with the following snippet.

from typing import Tuple
import numpy as np
from mlserver.codecs.decorator import SignatureCodec

def predict_fn_typing_tuple() -> Tuple[np.ndarray, np.ndarray]:
    return (np.array([4]), np.array([2]))


def predict_fn_native_tuple() -> tuple[np.ndarray, np.ndarray]:
    return (np.array([4]), np.array([2]))


def predict_fn_tuple_object() -> (np.ndarray, np.ndarray):
    return (np.array([4]), np.array([2]))


def test_typing_tuple():
    signature_codec = SignatureCodec(predict_fn_typing_tuple)
    assert signature_codec._output_hints == [np.ndarray, np.ndarray]


def test_native_tuple():
    signature_codec = SignatureCodec(predict_fn_native_tuple)
    assert signature_codec._output_hints == [np.ndarray, np.ndarray]


def test_tuple_object():
    signature_codec = SignatureCodec(predict_fn_tuple_object)
    assert signature_codec._output_hints == [np.ndarray, np.ndarray]

Please note: The notation for the predict_fn_native_tuple test requires Python 3.9. The PR code, however, does not depend on Python 3.9.

@adriangonz
Copy link
Contributor

Nice one! Thanks a lot for following up with this PR @sauerburger .

Changes look great! 🚀

For the tests, perhaps you can add a new one for the _as_list method, covering the different ways to specify the type hint? We run tests in 3.8 - 3.10, so we'll need to skip the tuple[np.ndarray, np.ndarray] form unfortunately. Happy to help out writing the test if you've got any questions.

Copy link
Contributor

@adriangonz adriangonz left a comment

Choose a reason for hiding this comment

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

Nice one @sauerburger ! Thanks for making those changes.

PR looks great. Once tests are green, it should be good to go ahead. 👍

@sauerburger
Copy link
Contributor Author

sauerburger commented Apr 4, 2023

Thanks for the kind feedback. I've fixed the issue that triggered the linter.

@adriangonz adriangonz merged commit 92e6f77 into SeldonIO:master Apr 4, 2023
26 checks passed
@adriangonz adriangonz added this to In progress in v1.3.x via automation Apr 4, 2023
@adriangonz adriangonz moved this from In progress to Done in v1.3.x Apr 4, 2023
@adriangonz adriangonz removed this from Done in v1.3.x Apr 4, 2023
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.

decode_args with tuple return value
2 participants