-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Support arbitrary request data when using SmallWebRTC with bot runner #2890
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,13 @@ class SmallWebRTCRequest: | |
| restart_pc: Optional[bool] = None | ||
| request_data: Optional[Any] = None | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, data: dict): | ||
| """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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reason for allowing both options?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the main reason is not break our webSDK, which is currently sending it as "requestData". |
||
|
|
||
|
|
||
| @dataclass | ||
| class IceCandidate: | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
➕ . 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creates an inheritance issue, resulting in a TypeError.
The new
bodyparameter is initialized in the RunnerArguments while the the subclass has a non-default required parameterroom_url. To solve this, we would need to either:body(e.g. init=False) (this is breaking, I think)kw_only=True(Potentially breaking if args provided positionally)bodyin the SmallWebRTCRunnerArguments (not breaking)Maybe this is the best outcome?
This makes
bodya keyword-arg, but that eliminates ambiguity across transports. (It is semi-breaking though..)Any changes made here need to be made to
pipecatcloudtypes, as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Is it if you're using positional args?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: @aconchillo just proposed this change, coincidentally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markbackman I can see how this is breaking. I will wait for @aconchillo to input.
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
bodywas onRunnerArguments, for example, I would not have had to update the definition forSmallWebRTCRunnerArgumentsas part of this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. In the case of multi-transport, you can just use the
RunnerArgumentstype. This already works well and is employed in a number of our examples. Two different ways this is done:You would only use the specific type if you have a single transport type.