python: support asset serving #259

bryfox · 2025-02-28T17:57:28Z

This adds support for an asset handler in the Python SDK. It follows the general approach of services: we expect a single function that returns the bytes of the asset, and call this function in a separate thread.

I also renamed "Request" -> "ServiceRequest" here. Even though the asset request is only a single URI, I think this clarifies what the request is for. It also localizes classes in the documentation.

I added a basic example which will serve "package://" assets from the local directory. We could include URDF descriptions in the example itself to make this easier to see out of the box; I haven't done so yet.

linear · 2025-02-28T17:57:31Z

FG-10666 Support asset service in python

eloff

Looks good, maybe one possibility to simplify a little, but don't spend too long on it

python/foxglove-sdk-examples/ws-asset-server/main.py

eloff · 2025-02-28T22:22:21Z

python/foxglove-sdk/src/websocket_server.rs

+/// asset `bytes`. If the handler returns `None`, a "not found" message will be sent to the client.
+/// If the handler raises an exception, the stringified exception message will be returned to the
+/// client as an error.
+pub struct AssetServer {


I think this could be simpler if you structured it as a closure passed as the blocking callback (if we're not using the builder you'd need to construct it yourself to set it in server options.)

At least it does the spawning and arc for you.

I'm struggling to come up with the conversion for that. I'll merge this for now and take another look when I flesh out the URDF asset example.

jtbandes · 2025-02-28T22:59:11Z

python/foxglove-sdk/python/foxglove/__init__.py

-ServiceHandler = Callable[["Request"], bytes]
+ServiceHandler = Callable[["ServiceRequest"], bytes]
+
+AssetHandler = Callable[[str], Optional[bytes]]


Feels like we need some explanation of what slow/async implementations are expected to do. What if the user wants an implementation that returns Awaitable? is that allowed? Any threading notes we need to add?

(Motivating example: how do I fit a slow network call into this API?)

In python, you can do whatever you want, which is why we spawn the callback on one of tokio's blocking threads. They can block as long as they want without causing any harm.

I don't have much experience with async python - and I get the impression it's not widely used - but if you really want to invoke an async function from the asset handler callback, I presume there's a way to get a handle to the asyncio eventloop and call loop.run(my_async_fn) to block and wait for it to complete.

I added docs for this. The callback is run in a separate thread. We decided against supporting async python interfaces in v1.

They can block as long as they want without causing any harm.

If they invoke something like a network call, will that result in the GIL being unlocked during the network call?

Sure, just the same as if they had made the network call from any other context.

jtbandes · 2025-02-28T22:59:25Z

python/foxglove-sdk/python/foxglove/_foxglove_py/__init__.pyi

@@ -180,7 +180,9 @@ AnyParameterValue = Union[
    ParameterValue.Dict,
 ]

-class Request:
+AssetHandler = Callable[[str], Optional[bytes]]


still learning how this stuff is set up, but why do we need 2 definitions of this type?

I was wondering about that too. Feels like we should be able to do from .. import AssetHandler, ServiceHandler or so, to pull the type definition from the parent module.

I added a comment in the public module. Normally, that file imports everything from this stub, but we need to re-define the types for sphnix docs to work. I think this stub could probably import from the public file, but I don't think it should.

jtbandes · 2025-02-28T23:01:11Z

python/foxglove-sdk/src/websocket_server.rs

@@ -356,6 +360,12 @@ pub fn start_server(
        server = server.services(services.into_iter().map(PyService::into));
    }

+    if let Some(asset_handler) = asset_handler {
+        server = server.fetch_asset_handler(Box::new(AssetServer {


Naming wise, the "AssetServer" name makes me think this is a standalone thing and not something I would necessarily expect to be passed in as an "asset handler"

could we call it something with AssetHandler in the name (I see AssetHandler is taken though) – PyAssetHandler? CallbackAssetHandler?

I guess using @eloff's suggestion of a closure would also resolve this concern because it would no longer need a name :)

Renamed to CallbackAssetHandler for now; I'll see if I can figure out conversions when I revisit the examples. This name isn't exposed to the python side, so I'd like to avoid the Py prefix.

jtbandes · 2025-02-28T23:03:10Z

python/foxglove-sdk-examples/ws-asset-server/main.py

+
+    try:
+        while True:
+            # Send transforms for the model as needed, on a `FrameTransformsChannel`


stale comment?

This was a placeholder instruction to the user; they'll need to publish the frame transforms.

I'm going to follow up with a more complete example, at which point I'll remove this.

jtbandes · 2025-02-28T23:03:58Z

python/foxglove-sdk-examples/ws-asset-server/main.py

+            time.sleep(1)
+
+    except KeyboardInterrupt:
+        server.stop()


feels like this could be nice to have as a helper method on the server, like sleep_until_interrupted or something like that?

While it might be nice to have that sugar for these examples, I don't think it'd find much practical use in the real world. Most users of this SDK are going to start the server, maybe register server.stop() as a pyexit hook if they're feeling fastidious, and then go about their actual business.

gasmith

LG

gasmith · 2025-02-28T23:10:40Z

python/foxglove-sdk-examples/ws-asset-server/main.py

+            time.sleep(1)
+
+    except KeyboardInterrupt:
+        server.stop()


While it might be nice to have that sugar for these examples, I don't think it'd find much practical use in the real world. Most users of this SDK are going to start the server, maybe register server.stop() as a pyexit hook if they're feeling fastidious, and then go about their actual business.

gasmith · 2025-02-28T23:11:21Z

python/foxglove-sdk/python/foxglove/__init__.py

    Schema,
    Service,
+    ServiceRequest,


Good call on the rename.

gasmith · 2025-02-28T23:15:00Z

python/foxglove-sdk/python/foxglove/__init__.py

-ServiceHandler = Callable[["Request"], bytes]
+ServiceHandler = Callable[["ServiceRequest"], bytes]
+
+AssetHandler = Callable[[str], Optional[bytes]]


In python, you can do whatever you want, which is why we spawn the callback on one of tokio's blocking threads. They can block as long as they want without causing any harm.

I don't have much experience with async python - and I get the impression it's not widely used - but if you really want to invoke an async function from the asset handler callback, I presume there's a way to get a handle to the asyncio eventloop and call loop.run(my_async_fn) to block and wait for it to complete.

gasmith · 2025-02-28T23:17:06Z

python/foxglove-sdk/python/foxglove/_foxglove_py/__init__.pyi

@@ -180,7 +180,9 @@ AnyParameterValue = Union[
    ParameterValue.Dict,
 ]

-class Request:
+AssetHandler = Callable[[str], Optional[bytes]]


I was wondering about that too. Feels like we should be able to do from .. import AssetHandler, ServiceHandler or so, to pull the type definition from the parent module.

gasmith · 2025-02-28T23:17:48Z

python/foxglove-sdk/src/websocket_server.rs

+/// asset `bytes`. If the handler returns `None`, a "not found" message will be sent to the client.
+/// If the handler raises an exception, the stringified exception message will be returned to the
+/// client as an error.
+pub struct AssetServer {


Doesn't need to be pub.

rust/foxglove/src/websocket.rs

Co-authored-by: Jacob Bandes-Storch <[email protected]>

bryfox added 4 commits February 28, 2025 09:39

Support asset server in Python

1d75f5a

Make capability implicit

fdaa549

Add example

261634e

Add readme

622773f

bryfox added 3 commits February 28, 2025 09:59

Log error if no handler provided

6875b8a

Remove import

5d08b0e

Remove useless log

3c01763

bryfox requested review from gasmith and eloff February 28, 2025 18:16

Log status

e016a86

eloff approved these changes Feb 28, 2025

View reviewed changes

Merge branch 'main' into bryan/fg-10666-support-asset-service-in-python

8b233ba

jtbandes reviewed Feb 28, 2025

View reviewed changes

gasmith approved these changes Feb 28, 2025

View reviewed changes

bryfox added 3 commits February 28, 2025 15:20

Update docs

ac2969a

Rename

8486533

Private

001aadb

jtbandes approved these changes Feb 28, 2025

View reviewed changes

rust/foxglove/src/websocket.rs Outdated Show resolved Hide resolved

Update rust/foxglove/src/websocket.rs

ae192b3

Co-authored-by: Jacob Bandes-Storch <[email protected]>

bryfox merged commit e0aa193 into main Feb 28, 2025
36 checks passed

bryfox deleted the bryan/fg-10666-support-asset-service-in-python branch February 28, 2025 23:45

python: support asset serving #259

python: support asset serving #259

Uh oh!

Conversation

bryfox commented Feb 28, 2025

Uh oh!

linear bot commented Feb 28, 2025

Uh oh!

eloff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtbandes Feb 28, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtbandes Feb 28, 2025 •

edited

Loading