Add LiveKit to runner - #5297
Conversation
b117d26 to
f7daced
Compare
markbackman
left a comment
There was a problem hiding this comment.
LGTM!
Can you add changelog fragments?
Other than that, just a few questions. I'll have Claude take a look too.
| messages: list[LLMContextMessage] | ||
| run_llm: bool | None = None | ||
|
|
||
| def __str__(self): |
There was a problem hiding this comment.
Was this change intentional?
| dailyRoom: str | None | ||
| dailyToken: str | None | ||
| url: str | None | ||
| # livekitToken: str | None |
There was a problem hiding this comment.
Are these two commented out for a reason?
markbackman
left a comment
There was a problem hiding this comment.
Claude's pass over the branch, as promised. The core fix looks right to me: push_app_message now mirrors Daily exactly (broadcast_frame(<Transport>InputTransportMessageFrame, ...)), and outbound RTVI already worked since LiveKitOutputTransport.send_message handles the generic frame class the processor pushes. Ran tests/test_livekit_transport.py on the branch — 12 passed.
One thing worth catching before merge: this branch predates #5293, so it needs a rebase. Details inline — it'll merge cleanly, which is exactly why it's easy to miss.
Smaller notes not worth inline comments:
LiveKitRunnerArgumentsis missingurlin itsParameters:docstring (pipecat/runner/types.py:238) — pre-existing, but a cheap drive-by since you're in this area.- The
/livekitredirect puts the join token in a query string handed tomeet.livekit.io, so it lands in browser history and referrer. Fine for a dev-only route; might be worth a line in the docstring saying so.
| session_id=session_id, | ||
| ) | ||
| runner_args.cli_args = args | ||
| asyncio.create_task(bot_module.bot(runner_args)) |
There was a problem hiding this comment.
Needs a rebase on main. #5293 added _start_bot_session() (run.py:204) and converted every bot launch site to it, so background bot tasks keep a strong reference for the life of the session — a bare asyncio.create_task can be collected mid-call.
The merge is clean, so these two new sites would silently end up as the only unreferenced bot tasks left in the file. After rebasing:
| asyncio.create_task(bot_module.bot(runner_args)) | |
| _start_bot_session(bot_module.bot(runner_args)) |
| session_id=str(uuid.uuid4()), | ||
| ) | ||
| runner_args.cli_args = args | ||
| asyncio.create_task(bot_module.bot(runner_args)) |
There was a problem hiding this comment.
Same as above — _start_bot_session(bot_module.bot(runner_args)) after the rebase.
| return StartBotResult( | ||
| url=livekit_url, | ||
| token=user_token, | ||
| # room=room_name, |
There was a problem hiding this comment.
Third commented-out line, along with the two in StartBotResult. Since the response shape is a contract with the web transport PR, worth settling: the client can't learn the room name from this response today (it's only encoded in the token). If it doesn't need it, drop the line.
| room_name = os.getenv("LIVEKIT_ROOM_NAME") or f"pipecat-{uuid.uuid4().hex[:8]}" | ||
| # Distinct identities: the bot and the browser join as separate participants. | ||
| agent_token = generate_token_with_agent(room_name, "Pipecat Agent", api_key, api_secret) | ||
| user_token = generate_token(room_name, "User", api_key, api_secret) |
There was a problem hiding this comment.
The env validation and the two-token mint are duplicated verbatim between here and the /start branch — a small _livekit_config() helper returning (url, api_key, api_secret) plus a token-pair helper would collapse both.
Separately, the identities are constant ("User" / "Pipecat Agent"). With LIVEKIT_ROOM_NAME set, a second caller joins with the same identity as the first and LiveKit evicts the earlier participant. Suffixing with the session id would avoid a confusing failure mode.
| ) | ||
| print(" → Configure this URL in your Daily phone number settings") | ||
| elif args.transport == "livekit": | ||
| print("🚀 Bot ready! (LiveKit)") |
There was a problem hiding this comment.
This points at the root URL (the prebuilt UI), but until pipecat-prebuilt#52 lands the path that actually works is /livekit. Consider printing that until the prebuilt support ships.
There was a problem hiding this comment.
i really hope to have that land soon, so i'm going to leave as-is and remove a future todo to remove it again.
| try: | ||
| message = json.loads(data.decode()) | ||
| except (UnicodeDecodeError, json.JSONDecodeError) as e: | ||
| message = None |
There was a problem hiding this comment.
Two things here:
Non-dict JSON gets through. json.loads on "hi", 123, or false returns a non-dict that's still pushed downstream. RTVIProcessor._handle_transport_message does transport_message.get("label") inside a try/except ValidationError, so the AttributeError escapes to __internal_process_frame and turns into an error frame — one per stray message from any participant. Worth guarding:
if not isinstance(message, dict):
message = NoneSilent drop. e is unused and nothing is logged, so non-JSON payloads vanish with no trace. A logger.trace/debug here would save someone a debugging session. This is also a behavior change worth a changed fragment: data that isn't a JSON object no longer reaches the pipeline as a frame at all, only the on_data_received event.
| Args: (participant_id: str) | ||
| - on_participant_left: Called when a participant leaves. | ||
| Args: (participant_id: str, reason: str) | ||
| - on_client_connected: Called when a participant connects (alias for |
There was a problem hiding this comment.
The docstring promises Args: (participant: dict), but _on_participant_connected passes participant_id: str. Daily passes a Mapping[str, Any] (daily/transport.py:392).
Since the point of the alias is drop-in bot templates, a template that reads client["id"] will break on LiveKit. Either match Daily's shape or fix the docstring — right now it advertises the Daily contract and delivers a string.
There was a problem hiding this comment.
matching Daily...
| """Handle participant disconnected events.""" | ||
| await self._call_event_handler("on_participant_disconnected", participant_id) | ||
| await self._call_event_handler("on_participant_left", participant_id, "disconnected") | ||
| # Also call on_client_disconnected for compatibility with other transports |
There was a problem hiding this comment.
_on_participant_connected pushes a ClientConnectedFrame, but nothing pushes ClientDisconnectedFrame on the way out. Pre-existing, but runner support makes it a lot more visible — a follow-up issue if it's out of scope here.
There was a problem hiding this comment.
ClientDisconnectedFrame doesn't actually exist anywhere in the framework; Daily/SmallWebRTC/MoQ don't push anything symmetric to ClientConnectedFrame on disconnect either, so it's not a LiveKit-specific gap. bots handle disconnects via the on_client_disconnected event handler, which LiveKit fires correctly. Leaving out of scope here — happy to file a follow-up if we want a disconnect frame added across all transports.
| class TestLiveKitAppMessageInput(unittest.IsolatedAsyncioTestCase): | ||
| """Inbound data messages (RTVI's wire channel) must reach the pipeline. | ||
|
|
||
| Regression test: ``_on_data_received`` used to hand the raw undecoded |
There was a problem hiding this comment.
This narrates the previous implementation ("used to hand the raw undecoded string..."). AGENTS.md's writing-for-future-readers rule asks for what the code does now rather than what it used to do — the test reads fine if it just states the contract: inbound JSON data is parsed and broadcast both directions as an InputTransportMessageFrame so RTVIProcessor sees it wherever it sits in the pipeline.
|
Thanks for the notice, and that looks good to me as well in general! I'll close my PR when this is merged. But there's still one small point left, which might be out of scope of this PR, but was mentioned in #5218 as a side defect: currently this transport identifies participants by SID, but the Livekit APIs it calls resolve identity instead. This PR routes the inbound message correctly, but the value it carries as That line is where I noticed it, not where it ends. The transport emits The repo already contains the other choice. Reproduced on pipecat 1.7.0 against So the public API cannot consume its own output, and there is a second layer underneath: given the correct identity, these helpers reach bodies written against an older SDK surface, where Neither of these affects this PR. RTVI only broadcasts, so @markbackman: just flagging this, since it's outside this PR's scope and I'd rather hear whether it's worth an issue. The diff is small, the transport touches |
|
@XinZhou0417 -- i've been looking at other things today, but i'll look at the sid vs. participant_id tomorrow. thanks for raising that! |
| "body": {...} | ||
|
|
||
| // LiveKit-specific | ||
| "livekitRoomName": "my-room", |
There was a problem hiding this comment.
there's no equivalent to Daily of creating a room on the fly?
| room_name = ( | ||
| request_data.get("livekitRoomName") | ||
| or os.getenv("LIVEKIT_ROOM_NAME") | ||
| or f"pipecat-{uuid.uuid4().hex[:8]}" |
There was a problem hiding this comment.
Ah got it, so just omitting the livekitRoomName arg from the /start call (and not specifying an env var) causes it to fall back to creating a room.
That means you can't necessarily customize its behavior per request like (I assume) you can with Daily, having some requests use a new temporary room and some fall back to the env-var-supplied room. Am I understanding that right? If so: why make it work differently than Daily?
There was a problem hiding this comment.
im not sure i follow, because livekitRoomName is checked before the environment variable.. so i think it IS customizable per request. That said, your comment did bring something up. Daily actually is MORE constricting. It does not take a room name in the /start, so the only way to not use an auto-generated room is via an env variable. So maybe we should get rid of the livekitRoomName. I don't think my client changes support it and then we would work like Daily 😆
There was a problem hiding this comment.
So maybe we should get rid of the livekitRoomName. I don't think my client changes support it and then we would work like Daily 😆
Sounds good!
And actually...I misunderstood the purpose of "createDailyRoom". I thought it was a way of forcing /start to create a new Daily room regardless of whether it found a room in the env vars (DAILY_ROOM_URL), but that's not actually what it does!
So, yes, I'm with you! Making your suggested change would align the implementations.
| if not _transport_routes_enabled("livekit"): | ||
| return | ||
|
|
||
| @app.get("/livekit") |
There was a problem hiding this comment.
Why is this route valuable? (Same question goes for "/daily", actually).
I'll frame this question in terms of "/daily", but everything applies equally here. It seems like we have two main ways of starting the runner with Daily:
- The "direct" way, where we just print the Daily room URL for the user to navigate to (
-t daily -d) - The prebuilt client way, where the client uses the "/start" endpoint to start the bot and we print the prebuilt URL (
-t daily)
Then there's this odd, not-documented third way, which is through "/daily"...
There was a problem hiding this comment.
If we need something temporary here until we have LiveKit support on the Pipecat client, can't we just have folks use the "direct" way, where a LiveKit URL is printed that users can visit in their browser?
There was a problem hiding this comment.
i don't know. i was just following the daily standard here. it's probably a question for @markbackman as to whether we should consider removing these.
There was a problem hiding this comment.
Hmm, that's a weird one. I think we should not include the same for GET /livekit.
This allows you to type: http://localhost:7860/livekit to directly join a room. We should think about removing GET /daily too.
cc @filipi87 who added this when he made the dev runner more flexible. He might have had a motivation to add it.
There was a problem hiding this comment.
We should think about removing GET /daily too
Totally. Whether /daily is still necessary is my main question. Digging into history, /daily seems fairly old, and might be vestigial.
There was a problem hiding this comment.
It was originally at /, but moved to /daily three months ago in #4442.
I can't think of a reason to keep it. I never interact with it.
There was a problem hiding this comment.
What I don't want to lose in this thread: the alternative to /livekit that would be quite valuable is the direct link (the -t livekit -d invocation), mirroring Daily's direct link (see _run_daily_direct).
In other words, I'm not just suggesting removing /livekit, I'm suggesting replacing it with a direct link invocation.
There was a problem hiding this comment.
-d / --direct is a special case that we added for ourselves given how we used to test. I don't think we should port that for LiveKit.
We should add support for -t livekit though, which I believe is accounted for.
Though, the norm is for the client to specify the transport when connecting. In support of that Pipecat Prebuilt supports that pattern (and is now my preferred method for testing).
There was a problem hiding this comment.
given how we used to test
I test the direct way every day (it's still the fastest way, as far as I can tell). And I expect the direct route would be the fastest way to test with the LiveKit transport, too (I've tested our LiveKit transport with a browser tab on the LiveKit prebuilt UI).
But I concede that it's lower priority. It's just how I prefer to test (leaving a browser tab open forever).
f7daced to
b1cd0ad
Compare
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
kompfner
left a comment
There was a problem hiding this comment.
Looks good! Only remaining things on my end are:
- Removing
livekitRoomNameto match how Daily works (your suggestion) - (Optional) Adding support for
-d/--direct, which I personally would find quite helpful for my dev flow; I recognize @markbackman disagrees with this one
b6f5dc4 to
18f74f8
Compare
I removed livekitRoomName. as for the -d, claude convinced me to punt and we can support it later if we want. right now we heavily preference daily and i'm not 100% there is an equivalent easy direct link that livekit supports and will work. One new thing though. I made the updates per @XinZhou0417 's comments to update livekit to use identity which is the official id for users, not sid. ran some smoke tests and all seems good. |
| return url, api_key, api_secret | ||
|
|
||
|
|
||
| def _mint_livekit_tokens( |
There was a problem hiding this comment.
Should _mint_livekit_tokens() move to runner/livekit.py instead?
There was a problem hiding this comment.
Maybe as generate_session_tokens() or something along those lines?
| } | ||
|
|
||
|
|
||
| def _livekit_credentials() -> tuple[str, str, str]: |
There was a problem hiding this comment.
This one also feels out of place, but that's because there's no configure() equivalent option like Daily has. I'm not sure what the best plan is to align with it (or if we need to).
There was a problem hiding this comment.
Actually, maybe now is the time to adapt the runner/livekit.py's configure() to be more like Daily's. That could simplify things a bit. Anyway, something to think about before merging this.
seems reasonable
i am 100% sure. that's how i've tested livekit transport integration. which is why i was pushing for |
Touché. |
|
Well, Github is down, so the rest of my comment from above...
Everyone else will use Pipecat Prebuilt for this. |
|
Happy to add |
added it as a transport option to the development runner Fixes #5218
18f74f8 to
c443a05
Compare

This PR is an alternate fix for issue #5218 and tested alongside work being done to add a js client transport for LiveKit here: pipecat-ai/pipecat-client-web-transports#86
In addition to fixing the issue of the inbound message being sent back vs. broadcasted so that RTVI can handle it, it also adds LiveKit to the set of transports supported by the dev runner.
I haven't fully reviewed https://github.com/pipecat-ai/pipecat/pull/5224/changes which has been posted in parallel (sorry @XinZhou0417 -- i had already been working on this and didn't see your PR til I went to post this one). Let me know if your PR solves any issues I overlooked and feel free to add your review here.
Here is the complete list of corresponding PRs for getting LiveKit completely wired up throughout the pipecat ecosystem:
New Livekit client-side Transport PRs:
Corresponding Server fixes for LiveKit Transport:
Voice-Ui Kit support:
Pipecat Prebuilt: