python: implement parameters on server listener #231
Conversation
bryfox
force-pushed
the
bryan/fg-10186-python-allow-user-to-set-parameters
branch
from
887f0c3 to
46c6ddb
Compare
| request_id: Optional[str] = None, | ||
| ) -> list[Parameter]: | ||
| logging.debug(f"on_get_parameters: {param_names}, {client.id}, {request_id}") | ||
| return self.parameters |
There was a problem hiding this comment.
This is ignoring the param names, that seems like the wrong thing to do in an example
| request_id: str | None = None, | ||
| ) -> list[foxglove.Parameter]: | ||
| logging.debug(f"on_set_parameters: {parameters}, {client.id}, {request_id}") | ||
| existing_names = [p.name for p in self.parameters] |
There was a problem hiding this comment.
I think self.parameters should be a dict. See the rust example.
If you wanted to keep this as is you could use a set or dict comprehension.
Using list everywhere is simple but inefficient. I know it's only an example, but I like setting a good example.
There was a problem hiding this comment.
I originally wrote it with a dict, but thought that it got in the way of understanding the interface. I suppose having a lot of parameters could be typical; I'll update.
| initial_values: list[Parameter] = [ | ||
| Parameter( | ||
| "param1", | ||
| value=ParameterValue.Dict( |
There was a problem hiding this comment.
I see what you mean. This would be a much nicer interface in Python if you could just use native lists, dicts, bytes, etc and we did the conversion in our code.
Although we'd have to throw an exception at runtime if you passed the wrong type.
I feel like we could provide the correct typings though. Also this current code will still throw an exception at runtime if you pass the wrong types. So it's only slightly safer.
I guess for now this works and we can make it more ergonomic in a future iteration.
There was a problem hiding this comment.
I think it's reasonable to support both, so I'll keep this and have also filed FG-10589 for the alternative.
I updated the docs here; I filed FG-10592 for possibly renaming. |
bryfox
deleted the
bryan/fg-10186-python-allow-user-to-set-parameters
branch
This builds on #231 to add parameter subscription support to the Python SDK. The SDK takes care of all bookkeeping and this is purely informational for an implementer. This also removes "parametersSubscribe" from the public SDK. Because the SDK manages the subscriptions itself, as long as the client advertises the "parameters" capability, the SDK will automatically include the former capability. This does mean that the server listener callbacks would be called regardless, but both languages provide default no-op implementations.
This adds
on_get_parametersandon_set_parametersto the python server listener, along with an example.I was a bit surprised by the behavior of the latter, and only understood it by reading the ws-protocol spec; it works more like a patch interface. I updated the docs slightly to clarify this, but I wonder if we should consider renaming the callback for this. (Would we also consider implementing a parameter store internally in a future version?)
I think we'll eventually want a higher-level interface in Python for this too — dealing with 'type' classes is not ergonomic — but having a consistent interface seems valuable as well.