Handle parameterised generics #598

DiamondJoseph · 2024-08-20T12:48:57Z

Closes #597

codecov · 2024-08-20T12:54:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.99%. Comparing base (1d80837) to head (70108ea).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #598      +/-   ##
==========================================
+ Coverage   92.97%   92.99%   +0.01%     
==========================================
  Files          40       40              
  Lines        1823     1826       +3     
==========================================
+ Hits         1695     1698       +3     
  Misses        128      128

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ZohebShaikh · 2024-08-20T13:30:56Z

Nice @DiamondJoseph , I wanted to know if we can test the BlueAPI endpoints as part of this PR or should they be separate...It is so that we can have more confidence in our code that it is working

stan-dot

no idea what is going on

stan-dot · 2024-08-20T13:45:07Z

src/blueapi/service/runner.py

+def is_valid_return(value: Any, expected_type: type[T]) -> bool:
+    if expected_type is Any:
+        return True


it's not documented. when would we need this?

why do we use this 'rpc'?

We have 2 processes in blueapi: 1 exposes the REST API, and the other holds the RunEngine. The _rpc method in this file passes requests from the first to the second. As part of the validation and type hinting, the expected return type of the function is passed, just in case this has changed in the subprocess holding the RunEngine.

If the function that is to be run in the subprocess has a return annotation of Any, then whatever the value that is returned by running the function in the subprocess is, it is valid. If the return annotation is None, then the returned value is only valid if it is None.
The linked issue was exposed when returning a subscripted list, e.g. list[DeviceModel], as isinstance(foo, list[DeviceModel]) throws an exception. So this new code is a kind of isinstance that recursively finds the origin (list in the example) and args (DeviceModel in the example) and checks that the returned value is valid for both.

@stan-dot #590

can we just have no subprocesses?

#504

This PR makes #504 easier by making the subprocess work more like a separate RPC service...

504 isn't difficult in the first place imo, and this feels like reinventing the wheel

You may be right, but this is the smallest amount of work required to unblock people here and now

joeshannon · 2024-08-20T14:19:57Z

Nice @DiamondJoseph , I wanted to know if we can test the BlueAPI endpoints as part of this PR or should they be separate...It is so that we can have more confidence in our code that it is working

The endpoints themselves have tests (test_rest_api.py) however these are unit tests and don't test the subprocess, or actually running a plan etc.

runner.py has its own unit tests which could possibly be enhanced to catch this.

When refactoring in this area it took me a very long time to fix up the tests as they were all mocking various things across many levels of indirection including private components. The tests are now closer to actual unit tests.

What blueapi needs now is probably some form of system tests to confirm end to end functionality and results.
I would probably suggest that this would be done as part of a different change.

callumforrester · 2024-08-21T15:13:22Z

@DiamondJoseph Have written and pushed some test cases, all but one pass

callumforrester · 2024-08-22T07:52:35Z

src/blueapi/service/runner.py

+def is_valid_return(value: Any, expected_type: type[T]) -> bool:
+    if expected_type is Any:
+        return True
+    if expected_type is type(None):
+        return value is None
+    origin = get_origin(expected_type)
+    if origin is None or origin is Union:  # works for Python >= 3.10
+        return isinstance(value, expected_type)
+    args = get_args(expected_type)
+    if issubclass(origin, Mapping):
+        return isinstance(value, Mapping) and all(
+            is_valid_return(k, args[0]) and is_valid_return(v, args[1])
+            for k, v in value.items()
+        )
+    if origin is tuple:  # handle ellipses specially
+        assert isinstance(value, tuple)
+        if args[1] is Ellipsis:
+            args = (args[0],) * len(value)
+        else:
+            return all(
+                is_valid_return(inner, arg)
+                for inner, arg in zip(value, args, strict=True)
+            )
+    if issubclass(origin, Collection):  # list, set, etc.
+        return isinstance(value, Collection) and all(
+            is_valid_return(inner, args[0]) for inner in value
+        )
+    raise TypeError(f"Unknown origin for generic type {expected_type}")


Could pydantic.TypeAdapter do all of this?

If it can, I can't test until #449

@callumforrester with Pydantic.TypeAdapter it's still got an issue with the same single case. My handling is equally as good ;)

Shall we go with TypeAdapter and not support that case then? I don't see a use case for it currently.

callumforrester

LGTM!

callumforrester · 2024-08-29T12:12:40Z

src/blueapi/service/runner.py

-            f"{function_name} returned value of type {type(value)}"
-            + f" which is incompatible with expected {expected_type}"
-        )
+    return TypeAdapter(expected_type).validate_python(value)


Nit: Can this go in an else clause?

DiamondJoseph · 2024-08-29T12:18:04Z

I think pydantic/pydantic#6870 is the cause of our failing test case. In the debug console:

from typing import Generic, TypeVar
from pydantic import BaseModel, TypeAdapter

T = TypeVar("T")

class GenericModel(BaseModel, Generic[T]):
    t: T

g = GenericModel[int](t=1)
h = GenericModel(t=2)

ta = TypeAdapter(GenericModel)
ta2 = TypeAdapter(GenericModel[int])

ta.validate_python(g)
ta.validate_python(h)
ta2.validate_python(g)
# fails
ta2.validate_python(h)

As the return annotation of the function is ~GenericModel[int].

comments addressed

Closes #597 --------- Co-authored-by: Callum Forrester <callum.forrester@diamond.ac.uk>

DiamondJoseph requested review from ZohebShaikh, callumforrester, joeshannon and stan-dot August 20, 2024 12:48

DiamondJoseph force-pushed the param_generic branch 2 times, most recently from c01b2e0 to 7a78f59 Compare August 20, 2024 13:08

DiamondJoseph mentioned this pull request Aug 20, 2024

Tests for fastapi endpoints #600

Closed

stan-dot previously requested changes Aug 20, 2024

View reviewed changes

callumforrester reviewed Aug 22, 2024

View reviewed changes

DiamondJoseph force-pushed the param_generic branch from d969200 to 9ff47c8 Compare August 22, 2024 15:36

DiamondJoseph and others added 6 commits August 29, 2024 13:04

Handle parameterised generics

2cfbc47

correct tuple, set, Any and None handling

77dc8ba

Add tests for RPC return types

c4a90c4

Rebase on main

e9f38cd

Add issue link for known weakness of type validation

fbb2306

Remove partial special case

e1024e9

DiamondJoseph force-pushed the param_generic branch from 7f1445e to e1024e9 Compare August 29, 2024 12:04

callumforrester approved these changes Aug 29, 2024

View reviewed changes

if-else extra test case

70108ea

DiamondJoseph merged commit ad3f686 into main Aug 29, 2024
24 checks passed

DiamondJoseph deleted the param_generic branch August 29, 2024 12:27

ZohebShaikh pushed a commit that referenced this pull request Aug 29, 2024

Handle parameterised generics (#598)

3cd7945

Closes #597 --------- Co-authored-by: Callum Forrester <callum.forrester@diamond.ac.uk>

DiamondJoseph mentioned this pull request Aug 29, 2024

Replace references to blueapi messaging package #589

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle parameterised generics #598

Handle parameterised generics #598

DiamondJoseph commented Aug 20, 2024

codecov bot commented Aug 20, 2024 •

edited

Loading

ZohebShaikh commented Aug 20, 2024

stan-dot left a comment

stan-dot Aug 20, 2024

DiamondJoseph Aug 20, 2024

callumforrester Aug 21, 2024

stan-dot Aug 21, 2024

callumforrester Aug 21, 2024

stan-dot Aug 21, 2024 •

edited

Loading

callumforrester Aug 27, 2024

joeshannon commented Aug 20, 2024

callumforrester commented Aug 21, 2024

callumforrester Aug 22, 2024

DiamondJoseph Aug 22, 2024

DiamondJoseph Aug 22, 2024

callumforrester Aug 23, 2024

callumforrester left a comment

callumforrester Aug 29, 2024

DiamondJoseph commented Aug 29, 2024

Handle parameterised generics #598

Handle parameterised generics #598

Conversation

DiamondJoseph commented Aug 20, 2024

codecov bot commented Aug 20, 2024 • edited Loading

Codecov Report

ZohebShaikh commented Aug 20, 2024

stan-dot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stan-dot Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeshannon commented Aug 20, 2024

callumforrester commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

callumforrester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiamondJoseph commented Aug 29, 2024

codecov bot commented Aug 20, 2024 •

edited

Loading

stan-dot Aug 21, 2024 •

edited

Loading