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.99%. Comparing base (5d7e2fa) to head (5e8ee5b).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1469      +/-   ##
==========================================
+ Coverage   95.96%   95.99%   +0.03%     
==========================================
  Files          45       45              
  Lines        3323     3348      +25     
==========================================
+ Hits         3189     3214      +25     
  Misses        134      134              

☔ 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!

[tpoliaw]

This makes null appear in the schema where we are explicitly hiding it as an option (via SkipJsonSchema[NoneType] in _convert_type). If you want to expose the default values for the primitive cases, it would be better to keep the default_factory for the None case.

                match para.default:
                    case Parameter.empty:
                        info = FieldInfo(default_factory=None)
                    case None:
                        info = FieldInfo(default_factory=lambda: None)
                    case _:
                        info = FieldInfo(default=para.default)

[tpoliaw]

Move this dict to a _JSON_TYPE_MAP (or similar) global/constant

[tpoliaw]

This could be moved to the bottom of the module.

[tpoliaw]

Suggested change

	if "$ref" in schema:
	return schema["$ref"].split("/")[-1]
	if ref := schema.get("$ref"):
	return ref.split("/")[-1]

[tpoliaw]

Suggested change

	if "anyOf" in schema:
	return " | ".join(_pretty_type(s) for s in schema["anyOf"])
	if anyof := schema.get("anyOf"):
	return " | ".join(_pretty_type(s) for s in anyof)

[tpoliaw]

Suggested change

	item_schema = schema.get("items", {})
	inner = _pretty_type(item_schema)
	return f"list[{inner}]"
	items = schema.get("items", {})
	return f"list[{_pretty_type(items)}]"

[tpoliaw]

I think this could be simpler. required will be the same for every call so you could create it once before the function is defined and use it directly

required = set(self.required)
def _format_arg(name: str, info: dict[str, Any]) -> str:
    ...

is_required and has_default are only used in one place so can be inlined or merged with the default use

if name in required:
    return f"{name}: {typ}"
if default := info.get("default"):
    return f"{name}: {typ} = {default!r}"
return f"{name}: {typ} | None = None"

[tpoliaw]

If required is defined above and properties becomes a dict, this becomes

Suggested change

	args = [
	_format_arg(name, info, set(self.required)) for name, info in props.items()
	]
	args = [_format_arg(name, info) for name, info in self.properties.items()]

[tpoliaw]

I'm not sure what you can do with just the keys that you can't do with the whole dict so returning the full properties would be useful

[tpoliaw]

Suggested change

	plan.help_text == "Plan foo(\n"
	" one: Any,\n"
	" two: list[Any] | bool,\n"
	" three: Any = 3,\n"
	" four: Any | None = None\n"
	")"
	plan.help_text == dedent("""\
	Plan foo(
	one: Any,
	two: list[Any] | bool,
	three: Any = 3,
	four: Any | None = None
	)""")