Support arbitrary request data when using SmallWebRTC with bot runner #2890
Conversation
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
| self.handle_sigint = False | ||
| self.handle_sigterm = False | ||
| self.pipeline_idle_timeout_secs = 300 | ||
| self.body = self.body or {} |
There was a problem hiding this comment.
I think it makes sense to move the "body" here, since it seems it will be common to all transports and will allow receiving any custom data from the user.
And we will need to do the same inside the base image.
Tagging @mark for a second opinion on this, since he’s worked a lot more with the runners.
There was a problem hiding this comment.
➕ . Also, inconsistency in transport params for common properties like this can make building multi-transport bot files tricky. For me at least, this makes sense to have as a default.
There was a problem hiding this comment.
This creates an inheritance issue, resulting in a TypeError.
The new body parameter is initialized in the RunnerArguments while the the subclass has a non-default required parameter room_url. To solve this, we would need to either:
- defer initializing
body(e.g. init=False) (this is breaking, I think) - Make body
kw_only=True(Potentially breaking if args provided positionally) - Initialize
bodyin the SmallWebRTCRunnerArguments (not breaking)
Maybe this is the best outcome?
@dataclass
class RunnerArguments:
handle_sigint: bool = field(init=False)
handle_sigterm: bool = field(init=False)
pipeline_idle_timeout_secs: int = field(init=False)
body: dict[str, Any] = field(default_factory=dict, kw_only=True)
def __post_init__(self):
self.handle_sigint = False
self.handle_sigterm = False
self.pipeline_idle_timeout_secs = 300
This makes body a keyword-arg, but that eliminates ambiguity across transports. (It is semi-breaking though..)
Any changes made here need to be made to pipecatcloud types, as well.
There was a problem hiding this comment.
The least breaking option is to just add to SmallWebRTCRunnerArguments, but you keep the transport inconsistency (for positional args only).
@jptaylor what do you mean by:
Also, inconsistency in transport params for common properties like this can make building multi-transport bot files tricky.
Is it if you're using positional args?
There was a problem hiding this comment.
FYI: @aconchillo just proposed this change, coincidentally.
There was a problem hiding this comment.
@markbackman I can see how this is breaking. I will wait for @aconchillo to input.
Is it if you're using positional args?
Perhaps best to ignore input here as I'm clearly missing important context based on your comment (I didn't consider the positional args case, for example!) My point about multi-transport was that some bot methods can receive args that are either Daily or SmallWebRTC, so having a tight base class definition for request data seems like a good idea. If body was on RunnerArguments, for example, I would not have had to update the definition for SmallWebRTCRunnerArguments as part of this PR.
There was a problem hiding this comment.
Got it. In the case of multi-transport, you can just use the RunnerArguments type. This already works well and is employed in a number of our examples. Two different ways this is done:
- Without create_transport factory: https://github.com/pipecat-ai/pipecat-examples/blob/main/runner-examples/02-two-transport-bot.py#L104-L151
- With create_transport factory: https://github.com/pipecat-ai/pipecat-examples/blob/main/runner-examples/04-all-transport-factory-bot.py#L141-L144
You would only use the specific type if you have a single transport type.
jptaylor
changed the title
|
The From this PR, we still have:
|
| """Accept both snake_case and camelCase for the request_data field.""" | ||
| if "requestData" in data and "request_data" not in data: | ||
| data["request_data"] = data.pop("requestData") | ||
| return cls(**data) |
There was a problem hiding this comment.
What's the reason for allowing both options?
There was a problem hiding this comment.
I think the main reason is not break our webSDK, which is currently sending it as "requestData".
The
SmallWebRTCRequestdataclass expected Python snake_case field names (request_data), but the Pipecat JavaScript SDK passes a camelCase property (requestData) due to it's naturalAPIRequesttyping. This caused custom request data from JS clients to be silently ignored during deserialization.from_dict()classmethod toSmallWebRTCRequestthat maps requestData (camelCase) to request_data (snake_case) when present, enabling proper deserialization from both naming conventions.RunnerArgumentsbase class to supportbodywhich should be support by all current and future transport types.