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

Document how to handle non-serializable plan args #652

Open
DominicOram opened this issue Sep 26, 2024 · 2 comments
Open

Document how to handle non-serializable plan args #652

DominicOram opened this issue Sep 26, 2024 · 2 comments
Labels
documentation Improvements or additions to documentation worker Relates to worker code

Comments

@DominicOram
Copy link
Contributor

DominicOram commented Sep 26, 2024

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
Copy link
Collaborator

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.

@callumforrester callumforrester added enhancement New feature or request help wanted Extra attention is needed worker Relates to worker code labels Oct 8, 2024
@DominicOram
Copy link
Contributor Author

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

@callumforrester callumforrester added documentation Improvements or additions to documentation and removed enhancement New feature or request help wanted Extra attention is needed labels Oct 9, 2024
@callumforrester callumforrester changed the title Should BlueAPI expose all Optional args? Document how to handle non-serializable plan args Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation worker Relates to worker code
Projects
None yet
Development

No branches or pull requests

2 participants