-
Notifications
You must be signed in to change notification settings - Fork 150
Session API #789
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
base: main
Are you sure you want to change the base?
Session API #789
Conversation
6ea1621
to
2f9bbee
Compare
|
||
// MARK: - Init | ||
|
||
public init(credentials: CredentialsProvider, room: Room = .init(), agentName: String? = nil, senders: [any MessageSender]? = nil, receivers: [any MessageReceiver]? = nil) { |
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.
@1egoman @lukasIO I think that's the discussion about the logic:
agentName
should take part in the direct dispatch- we'll introduce plural case later while keeping an internal array? it creates some confusion why the conversation cannot happen "with multiple agents"
- wait for agents can only happen at the conversation level (as the
Agent
will be published when joining)- I believe we should check the names vs who actually joined
- name shouldn't take part in the filtering? so that I'll keep an agent that I formally did not pass?
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.
we'll introduce plural case later while keeping an internal array? it creates some confusion why the conversation cannot happen "with multiple agents"
This was generally what I had in mind yes, start with a singular agentName
-type parameter, and then in the future add an internal array which can be backed by a new agentNames
parameter (In swift I'd think it could probably be a new overload? On web, the way to accomplish the same thing would be that the parameter would now accept either a string
or Array<string>
, and renamed since parameter names aren't part of the external interface in js)
This link may be useful and represents the initial state of this on the web: https://github.com/livekit/components-js/pull/1207/files#diff-c2401cb9c778162d5def12d137a663f03477b02c804d9372846c318e903df77bR125-R146
wait for agents can only happen at the conversation level (as the Agent will be published when joining)
On the web right now, what I'm doing is in useConversation
I'm calling useAgent
to get access to the current agent, and then on the agent I exposed a method called waitForAvailable
which returns a promise which resolves once agent.isAvailable
is true. await agent.waitForAvailable()
is then being called in conversation.start()
.
Associated code here: https://github.com/livekit/components-js/pull/1207/files#diff-c2401cb9c778162d5def12d137a663f03477b02c804d9372846c318e903df77bR306-R346
In the future, what I had been thinking (and another use-case for conversation) is it could maintain a registry of all currently connected agents. I started going down the road of building that in javascript sort of with useAgentTimeoutStore
here (internal hook / not exposed). Right now it is largely responsible for agent timeout management but I could see that growing, storing data across multiple agents, and moving underneath conversation in the future.
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.
@1egoman I think like we're still scratching the surface with "absence" 🥲
My high-level approach (not implemented yet) would be:
- Conversation controls the (global) timeout (as it does the dispatch), agent does not know about its own timeout, etc. 🟢
- Currently, passing one
agentName
is a little misleading for people having multiple agents anyway (without direct dispatch)... - When
agentName
is passed:- we do wait for this particular agent to join
- should this wait be awaitable or background
- we do wait for this particular agent to join
- When multiple
agentNames
are passed:- do we wait for all?
- When no
agentName
is passed:- we do wait for any agent 🟠
- we do not wait at all?
Agent
is added to the registry when it joins the room (agent lifecycle == participant lifecycle), regardless of its (conversational)state 🟠 theoretically we can add them with"listening" | "thinking" | "speaking";
, maybe including"idle"
but it also limits the space of states (if we don't register them - with the guarantee that they won't disappear after turningidle
) or is a little confusing (if we register them with another concept of "availability"). IMO, consumers should learn whatAgentState
means for their use cases, it's hard to tell what "available" means universally.- I'm not sure if doing that
await agent.waitUntilAvailable(signal);
is a great idea - should it be awaitable if you still wanna present some placeholder UI while agent joins/dispatches; thisawait
can be modeled by agent's optionalityAgent?
/its tracks anyway 🟠 - How to model
iCanSpeak
state - which is crucial for the UI:- During pre-connect:
capturingAudio + .disconnected, .connecting, .reconnecting, .connected
- After pre-connect: probably
connectionState
(room) is not enough, but how to handle e.g. handoff - I'd rather interpret that as "I'm in the room and someone is listening" rather than "some agent X is listening" as there may be gaps, etc.
- During pre-connect:
The key difference is probably in how we think about "presence" - shall we make some arbitrary decisions here or no?
94ec7d0
to
e5caee2
Compare
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 generally looks good - lmk when the API is considered final
aa93417
to
212035c
Compare
51915ab
to
0c89008
Compare
c52f944
to
9b16217
Compare
|
ec9bdcb
to
ac90eb4
Compare
Sources/LiveKit/Agent/Session.swift
Outdated
receivers: receivers) | ||
} | ||
|
||
public convenience init(agentName: String, |
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 this is the best tradeoff with options, here agentName
takes priority and makes the intent clear (?)
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.
suggestion(non-blocking): The web agents sdk puts tokenSource
first because it was brought up in our meeting with ben/dz that the abstraction could be used outside of agent contexts as well. I see you have some other constructors above but if this is supposed to be an agent use case specific constructor pattern maybe it's worth making it a static method like Session.withAgent(agentName:agentMetadata:/* ... */)
or something like that?
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.
ah yes, factory method is even better 👍
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.
💌 9cf68ff
JS diff🟢 - good enough Session Lifecycle 🟢
Agent Representation 🟠
Messaging Pipeline 🟢
Timeout & Errors 🟠
Media & Local Resources 🟢
State Exposure & Reactivity 🟢
|
2074ccd
to
d5e6437
Compare
I haven't included most of the comments - until the API is approved 🟢 |
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.
Did another pass through and left a few notes, but largely looks like things are aligned on web + swift which is great!
Sources/LiveKit/Agent/Session.swift
Outdated
receivers: receivers) | ||
} | ||
|
||
public convenience init(agentName: String, |
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.
suggestion(non-blocking): The web agents sdk puts tokenSource
first because it was brought up in our meeting with ben/dz that the abstraction could be used outside of agent contexts as well. I see you have some other constructors above but if this is supposed to be an agent use case specific constructor pattern maybe it's worth making it a static method like Session.withAgent(agentName:agentMetadata:/* ... */)
or something like that?
} | ||
|
||
@Published public private(set) var agents: [Participant.Identity: Agent] = [:] |
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.
thought: You had mentioned in your comment here that js doesn't store a list of agents, only one right now - that is correct. The way I had been thinking about it, it's unclear if the agents sdk could even really support multiple agents right now (ie, ben had said that agents are unable to hear other agents, etc), so I opted to not implement this.
That being said, the external interfaces the web sdk provides leaves the door open for this in the future (ie, right now useSession(..., { agentName: 'foo' })
/ useAgent(session)
, and in the future something like useSession(..., { agents: [{agentName: 'foo'}] })
/ useAgent(session, 'foo')
, with useAgent(session)
becoming deprecated but returning the first agent that joined for backwards compatibility).
I think I slightly prefer what I did in the web sdk than what you did here but I am curious what you are thinking. I will say that I think whatever we decide, all SDKs should align on this so if we opt for fully supporting multiple agents I will make those changes on the web in a new pull request.
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.
My thinking was just this decision has to be made anyway, e.g. once you introduce this helper "hook" @LiveKitAgent
, the default behavior must change when introducing multiple agents.
I'd prefer to align all SDKs now because deprecation may lead to awkward patterns on some platforms.
Also, returning to this code after a while - I started thinking about generalizing that even more as dz mentioned - to participants, as the main entry point to the (legacy) room. Wdyt?
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.
At least right now on the web, the only maybe "awkward pattern" that would exist is that useAgent(session, /* no agent name */)
would return the first agent which joined. I don't see that as all that awkward at least on the web, and was thinking that when eventually multiple agents gets introduced more officially down the line the old useAgent(session, /* no agent name */)
signature could just be deprecated. But that's just for the web, other platforms may be more challenging.
This is to say - from an external interface perspective I don't think the decision here effects the web sdk that much. I'd like to get @lukasIO's perspective just to see if he has a different perspective, but it sounds like you're pushing for the web sdk internals to support multiple agents now, and I'm fine with doing that and keeping the same external web sdk interface for now.
Also, returning to this code after a while - I started thinking about generalizing that even more as dz mentioned - to participants, as the main entry point to the (legacy) room. Wdyt?
I'm not sure I understand what you are suggesting - can you write up an example?
defer { | ||
waitForAgentTask = Task { [weak self] in | ||
try await Task.sleep(nanoseconds: UInt64(timeout * Double(NSEC_PER_SEC))) | ||
try Task.checkCancellation() | ||
guard let self else { return } | ||
if connectionState == .connected, agents.isEmpty { | ||
self.error = .agentNotConnected | ||
} | ||
} | ||
} |
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.
thought: Just wanted to surface another thing you had mentioned in your comment here - I think you missed that the web implementation not only stores a list of failureReasons
but ALSO transitions to a new failed
state. That state transition IMO is the more important behavior.
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.
issue: Reading through this, it looks like it doesn't handle the case of multiple agents joining properly because it is one global task, even though your agents
data structure does handle multiple agents properly.
For example, I'm thinking of this scenario:
- Agent
a
is dispatched - Wait 10 seconds
- Agent
b
is dispatched - Wait 10 more seconds
- At this point, both agents are still in a non timed out state even though
a
hasn't connected after 20 seconds (the default timeout).
I think in practice this case won't ever be an issue right now because the external Session
interface won't let you do this, but IMO this bug shouldn't be left lurking when the plan is to allow behavior like this later, and I'd think it wouldn't be that hard to fix (start a timeout task for each agent independently).
Or alternatively, maybe it's worth just storing one agent internally for now like I mentioned in another comment, in which case what you are doing here I think works fine.
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.
suggestion: I think I prefer something closer to what I did on the web slightly to what you did here - what do you think about this:
- Add a
failure
case toAgentState
here. - Apply this state to any agent that doesn't join after the specified timeout in this above logic, along with maybe some sort of more detailed error into on the agent (maybe continue the
self.error
pattern onAgent
as well?) - I think it's fine to still store a value on session's
self.error
, as long as it's clear to implementers that the error means that "at least one agent didn't connect" and not "all agents didn't connect".
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 agree, I subconsciously avoided adding more states 😄
Agent
in SwiftUI world must be optional anyway, as it's injected via the magicalEnvironment
object, and we cannot just crash when it's not there- introducing the error state that you must handle (from the enum) just for the sake of the error scenario - maybe that's an overkill?
- IMO the core difference is I think more in terms of
Agent == Participant
- it's there where it's there, while you do more of anAgentPromise
kind of thing that may resolve with an error
My biggest argument here is still introducing more states that we need to handle sort of "manually" (are not just observations of the engine) may lead to awkward behaviors (like with the preconnect API when you need to "superimpose" your local state over the connection state).
I'll try to revisit the JS code, maybe find some consensus.
cc @lukasIO
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.
introducing the error state that you must handle (from the enum) just for the sake of the error scenario - maybe that's an overkill?
thought: My opinion - I see it as less overkill and more that without it, the agent state
value isn't properly representing all the options, so the Agent
ends up being a "leaky" state machine.
IMO the core difference is I think more in terms of Agent == Participant - it's there where it's there, while you do more of an AgentPromise kind of thing that may resolve with an error
thought: Yea you are right, on the web returning a "placeholder" agent response is important for a js-related reason (preserving the ability to destructure the useAgent
return). One other nice thing it unlocks is it means that if an end user queries an agent and gets back an empty value, they don't have to guess exactly what that "lack of value" means - the agent could be in the midst of dispatching, it could have not connected, etc. So with this approach you have to query somewhere else to get that state, and at least on the web, that state being "disconnected" from the agent object results in some ergonomic challenges (type narrowing via discriminated unions won't work properly).
My biggest argument here is still introducing more states that we need to handle sort of "manually" (are not just observations of the engine) may lead to awkward behaviors (like with the preconnect API when you need to "superimpose" your local state over the connection state).
question: I'm not exactly sure I understand the preconnect API nuance you are describing, can you ellaborate further?
thought: Reading this though, you are making me think that maybe your concerns would be alleviated by splitting up the AgentState
enum into two levels. Then you are never replacing a state value from lk.agent.state
with an "internal" state value. So something like:
type AgentState =
| { state: "connecting" }
| {
state: "connected",
agent: {
state: 'initializing' | 'idle' | 'listening' | 'thinking' | 'speaking',
/* todo: add other agent related metadata in here */
}
| {
state: "failed",
failureReasons: Array<string>, // Or maybe something else, just some way to capture the "why" behind the failure
}
(sorry for the typescript and not swift, I'm hoping that gets the point across!)
Adds 3 basic building blocks for simple(r) agent experiences:
Session
- connection, pre-connect, agent dispatch, agent filtering (e.g. by name), all agents, messages (broadcasted and aggregated for now)Agent
- wrapper aroundParticipant
, knows its tracks and internal stateLocalMedia
- (unrelated) helper to deal with local tracks in SwiftUIExample: livekit-examples/agent-starter-swift#29