feat: Improve BlueapiClient to add plan parameter type hints

[oliwenmandiamond]

Fixes #1505

Improves the help text for a plan to add the type hints and correct default values.

Before change:

>>> bc.plans.grid_analyserscan
grid_analyserscan(analyser, sequence, detectors, args, snake_axes=None, md=None)

>>> bc.plans.count
count(detectors, num=None, delay=None, metadata=None)

With this change

>>> bc.plans.grid_analyserscan
grid_analyserscan(
    analyser: ElectronAnalyserDetector,
    sequence: AbstractBaseSequence,
    detectors: list[Readable],
    args: Any,
    snake_axes: list[Any] | bool | None = None,
    md: dict | None = None
)

>>> bc.plans.count
count(
    detectors: list[Readable],
    num: int = 1,
    delay: float = 0,
    metadata: dict | None = None
)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.94%. Comparing base (a7c7d93) to head (a163bc5).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1469      +/-   ##
==========================================
+ Coverage   95.91%   95.94%   +0.03%     
==========================================
  Files          44       44              
  Lines        3255     3280      +25     
==========================================
+ Hits         3122     3147      +25     
  Misses        133      133              

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Alexj9837]

looks good, made two comments and maybe add a test for the added value

[tpoliaw]

I'm not sure about this. I agree that the current repr isn't great but trying to perfectly replicate the function signature is going to lead to edge cases where things don't behave, for instance this gives detectors: list[Readable] for an argument that expects a list of strings (or DeviceRefs). (Possibly fixed by #1121?)

I think this is coming back to a python function not being a perfect fit for an API endpoint. For count, we want group to be optional but we don't want the default to be in the signature (as it makes the schema invalid). I think it would be better if the repr for plans stopped trying to be a signature and became something like Plan("count", (movable, value, group, wait)) and the type/default information was in the description (available via help(bc.plans.count)) as it is already.

If the type stubs PR is merged it could go some of the way towards improving usability. For the 'optional but no default' cases we could have an Unspecified object or similar.

@EmsArnold, you've used the client in a repl a fair bit as well - any thoughts on usability vs correctness?

[tpoliaw]

I think we want to be careful that we're not making an invalid schema. With these changes the schema contains several fields such as

"group": {
    "title": "Group",
    "type": "string",
    "default": null
},

which in turn causes your repr to contain parameters such as metadata: dict = None which is not correctly typed.

[oliwenmandiamond]

I'm not sure about this. I agree that the current repr isn't great but trying to perfectly replicate the function signature is going to lead to edge cases where things don't behave, for instance this gives detectors: list[Readable] for an argument that expects a list of strings (or DeviceRefs). (Possibly fixed by #1121?)

I think this is coming back to a python function not being a perfect fit for an API endpoint. For count, we want group to be optional but we don't want the default to be in the signature (as it makes the schema invalid). I think it would be better if the repr for plans stopped trying to be a signature and became something like Plan("count", (movable, value, group, wait)) and the type/default information was in the description (available via help(bc.plans.count)) as it is already.

If the type stubs PR is merged it could go some of the way towards improving usability. For the 'optional but no default' cases we could have an Unspecified object or similar.

So this is something we should probably discuss in next drop in session. This is the balance we need to get right because you're right that the API endpoint doesn't seem to map to python typing very well and depends on what direction we want to head.

I think a compromise I could do with this change is not address the typing of devices, leave that to another change and instead handle primitive typing and default e.g

>>> bc.plans.count
count(
    detectors,
    num: int = 1,
    delay: float = 0,
    metadata: dict | None = None
)

I think the client plan typing should be derived from the blueapi schema rather than the original plan annotations. I think maybe we could it like this (in another change)

>>> bc.plans.count
count(
    detectors: DeviceRef[Readable],
    num: int = 1,
    delay: float = 0,
    metadata: dict | None = None
)

This way, it is not lying to the user using the client. They can clearly see if a device is Readable by checking protocols.

bc.devices.my_device.model.protocols

We could add a shortcut property for protocols

>>> bc.devices.my_device.protocols
["Readable", "Triggerable"]

Maybe we could also add some helpful methods to find specific protocols

>>> bc.print_readables()
["motor_x", "detector", ...]

>>> bc.print_triggerables()
["detector", ...]

I also think this should apply for specific server types as well e.g using a Motor and a StandardDetector plan

>>> bc.plans.my_plan
my_plan(
    motor: DeviceRef[Motor],
    detector: DeviceRef[StandardDetector],
)

Then we can also implement a new property for DeviceRef called server_type

>>> bc.devices.x.server_type
"Motor"

We could also override the str of device ref to be a little more useful

>>> bc.devices.x
"Device(name=x, server_type=Motor)"

So to reply to your comments, I think what I've done is not quite right but can still be reused for primitive types and defaults. I think the idea of Plan("count", (movable, value, group, wait)) is not the right solution either as this still offers no useful information to the user.

A couple ideas to think about on how best to address this. Let me know what you think!