DiamondLightSource / blueapi

Apache License 2.0
6 stars 6 forks source link

Document how to handle non-serializable plan args #652

Open DominicOram opened 2 months ago

DominicOram commented 2 months ago

On i04 we have two plans that we want GDA to be able to call (plan_a and plan_b). plan_a is basically a super-set of plan_b except we want to inject a bit of extra logic into the middle of plan_b. I tried to implement this as:


def plan_a() -> MsgGenerator:
     ...
     def subplan():
          ...
     yield from plan_b(subplan)

def plan_b(subplan:Callable[[], MsgGenerator]=None) -> MsgGenerator:
    ...
    if subplan:
        yield from subplan()
    ...

However, I go the error of:

``` Traceback (most recent call last): File "/venv/lib/python3.11/site-packages/uvicorn/protocols/http/h11_impl.py", line 406, in run_asgi result = await app( # type: ignore[func-returns-value] ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/venv/lib/python3.11/site-packages/uvicorn/middleware/proxy_headers.py", line 70, in __call__ return await self.app(scope, receive, send) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/venv/lib/python3.11/site-packages/fastapi/applications.py", line 1054, in __call__ await super().__call__(scope, receive, send) File "/venv/lib/python3.11/site-packages/starlette/applications.py", line 113, in __call__ await self.middleware_stack(scope, receive, send) File "/venv/lib/python3.11/site-packages/starlette/middleware/errors.py", line 187, in __call__ raise exc File "/venv/lib/python3.11/site-packages/starlette/middleware/errors.py", line 165, in __call__ await self.app(scope, receive, _send) File "/venv/lib/python3.11/site-packages/starlette/middleware/base.py", line 185, in __call__ with collapse_excgroups(): File "/usr/local/lib/python3.11/contextlib.py", line 158, in __exit__ self.gen.throw(typ, value, traceback) File "/venv/lib/python3.11/site-packages/starlette/_utils.py", line 83, in collapse_excgroups raise exc File "/venv/lib/python3.11/site-packages/starlette/middleware/base.py", line 187, in __call__ response = await self.dispatch_func(request, call_next) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/venv/lib/python3.11/site-packages/blueapi/service/main.py", line 342, in add_api_version_header response = await call_next(request) ^^^^^^^^^^^^^^^^^^^^^^^^ File "/venv/lib/python3.11/site-packages/starlette/middleware/base.py", line 163, in call_next raise app_exc File "/venv/lib/python3.11/site-packages/starlette/middleware/base.py", line 149, in coro await self.app(scope, receive_or_disconnect, send_no_error) File "/venv/lib/python3.11/site-packages/starlette/middleware/exceptions.py", line 62, in __call__ await wrap_app_handling_exceptions(self.app, conn)(scope, receive, send) File "/venv/lib/python3.11/site-packages/starlette/_exception_handler.py", line 62, in wrapped_app raise exc File "/venv/lib/python3.11/site-packages/starlette/_exception_handler.py", line 51, in wrapped_app await app(scope, receive, sender) File "/venv/lib/python3.11/site-packages/starlette/routing.py", line 715, in __call__ await self.middleware_stack(scope, receive, send) File "/venv/lib/python3.11/site-packages/starlette/routing.py", line 735, in app await route.handle(scope, receive, send) File "/venv/lib/python3.11/site-packages/starlette/routing.py", line 288, in handle await self.app(scope, receive, send) File "/venv/lib/python3.11/site-packages/starlette/routing.py", line 76, in app await wrap_app_handling_exceptions(app, request)(scope, receive, send) File "/venv/lib/python3.11/site-packages/starlette/_exception_handler.py", line 62, in wrapped_app raise exc File "/venv/lib/python3.11/site-packages/starlette/_exception_handler.py", line 51, in wrapped_app await app(scope, receive, sender) File "/venv/lib/python3.11/site-packages/starlette/routing.py", line 73, in app response = await f(request) ^^^^^^^^^^^^^^^^ File "/venv/lib/python3.11/site-packages/fastapi/routing.py", line 301, in app raw_response = await run_endpoint_function( ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/venv/lib/python3.11/site-packages/fastapi/routing.py", line 214, in run_endpoint_function return await run_in_threadpool(dependant.call, **values) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/venv/lib/python3.11/site-packages/starlette/concurrency.py", line 39, in run_in_threadpool return await anyio.to_thread.run_sync(func, *args) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/venv/lib/python3.11/site-packages/anyio/to_thread.py", line 56, in run_sync return await get_async_backend().run_sync_in_worker_thread( ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/venv/lib/python3.11/site-packages/anyio/_backends/_asyncio.py", line 2177, in run_sync_in_worker_thread return await future ^^^^^^^^^^^^ File "/venv/lib/python3.11/site-packages/anyio/_backends/_asyncio.py", line 859, in run result = context.run(func, *args) ^^^^^^^^^^^^^^^^^^^^^^^^ File "/venv/lib/python3.11/site-packages/blueapi/service/main.py", line 110, in get_plans return PlanResponse(plans=runner.run(interface.get_plans)) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/venv/lib/python3.11/site-packages/blueapi/service/runner.py", line 91, in run return self._run_in_subprocess(function, *args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/venv/lib/python3.11/site-packages/blueapi/service/runner.py", line 112, in _run_in_subprocess return self._subprocess.apply( ^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/multiprocessing/pool.py", line 360, in apply return self.apply_async(func, args, kwds).get() ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/multiprocessing/pool.py", line 774, in get raise self._value pydantic.errors.PydanticInvalidForJsonSchema: Cannot generate a JsonSchema for core_schema.CallableSchema ```

It's understandable that a Callable cannot be used in a pydantic model and supplied by GDA but I never need to supply subplan from GDA. This can be solved by refactoring into a 3rd plan that the two both call but we could also have a general rule of if an arg is optional and not serialiasable then do not worry about exposing it? Probably with a message to the user

callumforrester commented 1 month ago

Some thoughts, let me know what you think...

I think detecting special cases where serialization is not needed adds some complexity, both to the code and to the UX. As you say we'd need a message for the user and to hope they read it when they're bashing their head against their plan wondering why this one specific parameter doesn't show up.

My preference in this case would be to not expose plan_b to blueapi at all. The idea is that you expose plans that part of the "beamline API", i.e. the set of operations that take serializable parameters. Depending on what you've got in-concrete you might need to refactor into a 3rd plan to make it work.

It's possible we need more fine-grained control over which plans are exposed and which are not to achieve this, my initial, simple plan was that you'd just have a module where you put all the top-level API plans and put all the rest somewhere else. Or that you'd prefix your 3rd plan with an underscore.

DominicOram commented 1 month ago

I'm happy with that decision, can we change this issue to just be to document it somewhere?