Support parameter subscriptions #238

bryfox · 2025-02-24T21:57:23Z

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.

linear · 2025-02-24T21:57:27Z

FG-10183 [python] add parameter callbacks to listener

bryfox · 2025-02-24T22:39:51Z

In the example here, when I'm connected to the app and edit a value, I see duplicated messages with the updated parameters. I think we're notifying the client as the originator of the new parameters, and also as a subscriber. This isn't a big deal, but is probably unnecessary; I wonder if we should skip the former if subscribed.

eloff · 2025-02-24T23:57:39Z

This builds on #231 to add parameter subscription support to the Python SDK.

As I understand it, the SDK takes care of all bookkeeping and this is purely informational for an implementer.

For discussion

As-is, the callback does not seem particularly useful: as a caller, I'm given a list of parameter names and no other information. I think we could consider either (a) including a client ID in the callback, or (b) removing these callbacks entirely. I think (a) makes the callback potentially meaningful, but risks a user thinking they need to implement bookkeeping.

So these callbacks notify when a parameter is first subscribed to our last unsubscribed to. Which is why I removed the client from the signature. Which client triggered that doesn't really matter and might confuse the user into thinking they have to do the bookkeeping.

The idea is it lets you know when you need to start being concerned about a parameter and when you can stop.

In any case, I think we should also consider automatically advertising the ParametersSubscribe support if the user specifies the Parameters capability, since the SDK itself is capable of managing the subscription. In other words, we only surface the Parameters capability to users of the SDK, and deal with the other one internally.

Yeah, that makes sense to me.

eloff · 2025-02-24T23:58:35Z

In the example here, when I'm connected to the app and edit a value, I see duplicated messages with the updated parameters. I think we're notifying the client as the originator of the new parameters, and also as a subscriber. This isn't a big deal, but is probably unnecessary; I wonder if we should skip the former if subscribed.

That's a good point. I'm not sure the ws-protocol spec lets us skip the response though? I wonder what we should do here.

eloff

This looks good, but could use some python tests or be added to the params example so it has some usage.

bryfox · 2025-02-25T16:00:55Z

So these callbacks notify when a parameter is first subscribed to our last unsubscribed to.

OK, that makes sense, and I agree with omitting the client ID; thank you

The idea is it lets you know when you need to start being concerned about a parameter and when you can stop.

I don't quite understand the use case here, though. Why would a server only be concerned about a parameter if a client is subscribed to it? Clients could be polling with getParameters .

bryfox · 2025-02-25T16:04:55Z

I'm not sure the ws-protocol spec lets us skip the response though? I wonder what we should do here.

There is a difference in the two messages today, since one of them has an ID. I think we should leave it alone as it's not going to cause problems, but could look at optimizing it down the line. This may evolve away from ws-protocol anyway.

…s-to-listener

bryfox · 2025-02-25T17:22:27Z

This looks good, but could use some python tests or be added to the params example so it has some usage.

I updated the example so it sort of explains how the subscription callbacks work. I also added a very basic unit test that includes the listener subscription calls; more to assert that the listener isn't abstract.

I've also removed the "parametersSubscribe" per discussion above; see updated PR decsription.

gasmith

LG, just some minor comments.

gasmith · 2025-02-25T17:59:11Z

rust/foxglove/src/websocket/protocol/server.rs

@@ -138,16 +138,13 @@ pub struct RemoveStatus {
 }

 /// A capability that the websocket server advertises to its clients.
-#[derive(Debug, Serialize, Eq, PartialEq, Hash)]
+#[derive(Debug, Serialize, Eq, PartialEq, Hash, Clone)]


Make it Copy too, while you're at it.

gasmith · 2025-02-25T18:10:15Z

python/foxglove-sdk/src/websocket_server.rs

+        let result: PyResult<()> = Python::with_gil(|py| {
+            let args = (param_names,);
+            self.listener
+                .bind(py)
+                .call_method("on_parameters_unsubscribe", args, None)?;
+
+            Ok(())
+        });


@eloff this is an example of why I feel uncomfortable about holding locks while making callbacks. In this context, the client poll loop is holding the server.subscribed_parameters mutex, and now we have to grab the GIL in order to invoke the python callback implementation.

If we ever have a case where a call from python might acquire the server.subscribed_parameters mutex (we don't have such a case today), that's an AB/BA deadlock waiting to happen (unless we're careful about dropping the GIL first).

gasmith · 2025-02-25T18:14:49Z

rust/foxglove/src/websocket/protocol/server.rs

@@ -159,17 +156,36 @@ pub enum Capability {
    Services,
 }

+/// ws-protocol includes a "parametersSubscribe" capability in addition to "parameters".
+/// Because the SDK handles subscription management internally, we only expose the latter publicly.
+#[derive(Debug, Serialize, Eq, PartialEq, Hash)]


Might as well make this Clone & Copy too.

gasmith · 2025-02-25T18:16:45Z

rust/foxglove/src/websocket/protocol/server.rs

@@ -138,16 +138,13 @@ pub struct RemoveStatus {
 }

 /// A capability that the websocket server advertises to its clients.
-#[derive(Debug, Serialize, Eq, PartialEq, Hash)]
+#[derive(Debug, Serialize, Eq, PartialEq, Hash, Clone)]
 #[serde(rename_all = "camelCase")]
 pub enum Capability {


With this change, it starts to feel like this more abstracted Capability should move up to crate::websocket, since it is no longer strictly part of the protocol.

bryfox requested review from gasmith and eloff February 24, 2025 21:58

eloff reviewed Feb 24, 2025

View reviewed changes

Base automatically changed from bryan/fg-10186-python-allow-user-to-set-parameters to main February 25, 2025 14:44

Support parameter subscriptions

1d7d287

bryfox force-pushed the bryan/fg-10183-python-add-parameter-callbacks-to-listener branch from ed96314 to 1d7d287 Compare February 25, 2025 14:59

bryfox added 4 commits February 25, 2025 08:23

Update example & tests

d29039c

Remove ParameterSubscribe from public docs

8af3581

Merge branch 'main' into bryan/fg-10183-python-add-parameter-callback…

8705e22

…s-to-listener

No Client constructor

419dfc7

bryfox requested a review from eloff February 25, 2025 17:22

gasmith approved these changes Feb 25, 2025

View reviewed changes

bryfox added 2 commits February 25, 2025 11:05

Move capability to parent

bb83065

No need to clone

ad622df

bryfox merged commit be31a49 into main Feb 25, 2025
29 checks passed

bryfox deleted the bryan/fg-10183-python-add-parameter-callbacks-to-listener branch February 25, 2025 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support parameter subscriptions #238

Support parameter subscriptions #238

Uh oh!

bryfox commented Feb 24, 2025 •

edited

Loading

Uh oh!

linear bot commented Feb 24, 2025

Uh oh!

bryfox commented Feb 24, 2025

Uh oh!

eloff commented Feb 24, 2025

For discussion

Uh oh!

eloff commented Feb 24, 2025

Uh oh!

eloff left a comment

Uh oh!

bryfox commented Feb 25, 2025

Uh oh!

bryfox commented Feb 25, 2025

Uh oh!

bryfox commented Feb 25, 2025

Uh oh!

gasmith left a comment

Uh oh!

gasmith Feb 25, 2025

Uh oh!

gasmith Feb 25, 2025 •

edited

Loading

Uh oh!

gasmith Feb 25, 2025

Uh oh!

gasmith Feb 25, 2025

Uh oh!

Uh oh!

Uh oh!

Support parameter subscriptions #238

Support parameter subscriptions #238

Uh oh!

Conversation

bryfox commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linear bot commented Feb 24, 2025

Uh oh!

bryfox commented Feb 24, 2025

Uh oh!

eloff commented Feb 24, 2025

For discussion

Uh oh!

eloff commented Feb 24, 2025

Uh oh!

eloff left a comment

Choose a reason for hiding this comment

Uh oh!

bryfox commented Feb 25, 2025

Uh oh!

bryfox commented Feb 25, 2025

Uh oh!

bryfox commented Feb 25, 2025

Uh oh!

gasmith left a comment

Choose a reason for hiding this comment

Uh oh!

gasmith Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

gasmith Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasmith Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

gasmith Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bryfox commented Feb 24, 2025 •

edited

Loading

gasmith Feb 25, 2025 •

edited

Loading