quarkiverse / quarkus-langchain4j

Quarkus Langchain4j extension
https://docs.quarkiverse.io/quarkus-langchain4j/dev/index.html
Apache License 2.0
128 stars 77 forks source link

Proper CDI scope for conversation #47

Open cescoffier opened 9 months ago

cescoffier commented 9 months ago

Memory is required to keep tracks of the previous messages in a conversation. It's also required when using tools.

This issue is about addressing two issues:

Of course, the memory storage must be per conversation / users and not for the application.

An idea is to define a new CDI scope that would handle the second issue.

geoand commented 9 months ago

We need this very very much :)

geoand commented 9 months ago

Let's start with a specific implementation of memory for Websockets and see how far that gets us.

agoncal commented 9 months ago

What about calling it @ConversationScoped ;o)

cescoffier commented 9 months ago

@agoncal That's exactly the problem. In addition,

1) we would need "session" support (Ladislav is working on this). 2) a user may start multiple conversations concurrently, making things slightly more complicated.

mkouba commented 9 months ago

I believe that a new well-defined scope would be more appropriate. I do remember the hard times trying to fix all the bugs related to the conversation context in Weld.

geoand commented 9 months ago

@mkouba just so we can be aware, what sort of issues where there? Asking because I am sure similar problems will arise in this context

mkouba commented 9 months ago

@mkouba just so we can be aware, what sort of issues where there? Asking because I am sure similar problems will arise in this context

Well, I don't remember all the details but I can try to walk through the Weld issues/codebase in order to identify the problematic parts. I do remember that many issues were related to concurrent access and lifecycle (such as "an HTTPSession is destroyed in the middle of a request processing"). We also had to initialize the context lazily because of some character encoding issues (see http://weld.cdi-spec.org/documentation/#3). You also need a mechanism to prune long-running conversations that were not explicitly ended (i.e. timed out). Anyway, the conversation context is bound to the Servlet API. So at least it would be very confusing if we support this scope on top of Vert.x.

geoand commented 9 months ago

Thanks for the info!

exabrial commented 9 months ago

FYI, there is already a CDI Scope called ConversationScoped, may want to avoid a name collision

geoand commented 9 months ago

Yeah, we definitely don't want more confusion

maxandersen commented 9 months ago

how to properly define the boundary of the memory - like the web socket session

is this not similar/exact same to our struggles around scoping various contexts when it comes to reactive call chains, http sessions, web socket, grpc, etc? i.e. websocket is not "special" in this is it?

maxandersen commented 9 months ago

Lets just for the value of my (and possible others) education, explore @ConversationScoped on why it makes sense and then lets see why it would not.

From CDI Spec I see:


    begin() marks the current transient conversation long-running. A conversation identifier may, optionally, be specified. If no conversation identifier is specified, an identifier is generated by the container.

    end() marks the current long-running conversation transient.

    getId() returns the identifier of the current long-running conversation, or a null value if the current conversation is transient.

    getTimeout() returns the timeout, in milliseconds, of the current conversation.

    setTimeout() sets the timeout of the current conversation.

    isTransient() returns true if the conversation is marked transient, or false if it is marked long-running.

Meaning - it's scope that you can have multiple of, can give it an id and it transcends multiple requests - seems like a perfect fit at first glance.

the conversation context is bound to the Servlet API.

Why is that ? If you refer to Conversation Context in Jakarta EE is that not just limited to Jakarta EE ? something we aren't bound by/limited by ?

as I understand it the CDI spec is not limited to jakarta EE nor is there a limit on how many @ConversationScope's we could have (even in Jakarta EE)?

agoncal commented 9 months ago

Adding @antoinesd as he also had some concerns about @ConversationScoped

mkouba commented 9 months ago

the conversation context is bound to the Servlet API.

Why is that ? If you refer to Conversation Context in Jakarta EE is that not just limited to Jakarta EE ? something we aren't bound by/limited by ?

The built-in implementation is bound to the Servlet API.

In theory, you could implement your own conversation context but there are obstacles. First of all, the built-in context (servlet-based) should be supported if you provide a custom one. Because users would be super confused if they have undertow extension (servlet) in their project but the lifecycle/behavior of the converstation context is completetly different (compared to the well-known built-in impl).

The container must provide a built-in bean for jakarta.enterprise.context.Conversation that would have to support the custom conversation context. That's not impossible but requires additional SPI so that the container can use the custom conversation context.

Futhermore, the Conversation API does not allow you to switch to another conversation as it expects that (in most cases) a single conversation is used in a request.

In any case, you would need to attach the conversation to some kind of "session". Which we currently don't have. And in my opinion it should not be based on a web technology because then it would not be usable outside an HTTP request anyway (which is the case for the built-in impl).

gavinking commented 9 months ago

Hi @mkouba

The built-in implementation is bound to the Servlet API.

That might be true, but I'm not sure why it matters?

In theory, you could implement your own conversation context

I mean, CDI was deliberately designed so that you can, so this is not just a matter of "theory", it how CDI was intended to be used.

but there are obstacles

So they should be removed.

First of all, the built-in context (servlet-based) should be supported if you provide a custom one.

Naturally, but how's this an "obstacle"?

Because users would be super confused if they have undertow extension (servlet) in their project but the lifecycle/behavior of the converstation context is completetly different (compared to the well-known built-in impl).

The lifecycle of the conversation context is defined by section 6.7.4-6.7.5 of the CDI spec. It is controlled by explicit user invocation of the Conversation interface. That section of the spec does not mention, and has never mentioned, anything about HTTP.

Section 11.3.4 outlines a particular implementation of a conversation context which is compatible with the Java EE web layer. That section of the spec is not supposed to be read as defining the behavior of the conversation context in general.

The container must provide a built-in bean for jakarta.enterprise.context.Conversation that would have to support the custom conversation context. That's not impossible but requires additional SPI so that the container can use the custom conversation context.

It's not only "not impossible", it should in principle be straightforward. If it's currently difficult, then that's a limitation we should address.

In any case, you would need to attach the conversation to some kind of "session". Which we currently don't have.

I mean, yes, of course. Naturally, that's something we would need to solve. I read that as being an implicit assumption in this proposal, not an objection to the proposal.

And in my opinion it should not be based on a web technology because then it would not be usable outside an HTTP request anyway (which is the case for the built-in impl).

Indeed it should not be limited on HTTP. That's implicit, I guess.

Futhermore, the Conversation API does not allow you to switch to another conversation as it expects that (in most cases) a single conversation is used in a request.

Right, There's indeed a subtlety we need to nail down here, which is that a transient conversation context is implicitly created and associated with every HTTP request. We would need to make sure that there's no room for pathological interaction between this transient context and the "chat" conversation. I have not thought about this very carefully, and there might indeed be some things to solve here. That said, as a matter of first impression, I don't see why it should not be doable in principle.

@mkouba @ConversationScoped is an abstract notion, just like @RequestScoped. Its definition is completely clean of everything to do with HTTP. Just like you can have a request context for a request that does not originate in HTTP, you can have a conversation that doesn't take place over HTTP.

gavinking commented 9 months ago

a user may start multiple conversations concurrently, making things slightly more complicated.

@cescoffier so the wrinkle here is: can the "user" start multiple conversations from within the same request context? If so, then there's indeed some sort of mismatch with how @ConversationScoped is currently conceptualized. But if you can only start a single conversation within a given request context, I think it's a good fit. And, FTR, I don't know how "multiple conversations per request context" would even work. Seems like something that would be quite hard to fit into the whole model of CDI. (Note that "request context" ≠ HTTP request, you could of course do something to fork off a new request context, and start a conversation from there.)

geoand commented 9 months ago

I mean, yes, of course. Naturally, that's something we would need to solve

Absolutely. In Quarkus currently we have a major gap in the WebSocket handling which I envision will be a heavily "transport" for such "conversations"

gavinking commented 9 months ago

Absolutely. In Quarkus currently we have a major gap in the WebSocket handling which I envision will be a heavily "transport" for such "conversations"

And that's even worth solving completely independently of this idea, right?

geoand commented 9 months ago

Totally

mkouba commented 9 months ago

In theory, you could implement your own conversation context

I mean, CDI was deliberately designed so that you can, so this is not just a matter of "theory", it how CDI was intended to be used.

No matter what was intended I've never seen a custom conversation context impl. Hence it's a theory from my point of view.

First of all, the built-in context (servlet-based) should be supported if you provide a custom one.

Naturally, but how's this an "obstacle"?

We deliberately chose not to implement this context (and it's not even required by CDI Lite) because we wanted to avoid all the problems we had to deal with in Weld during the years; corner cases like manual session invalidation, character encoding issues mentioned in https://github.com/quarkiverse/quarkus-langchain4j/issues/47#issuecomment-1842446440, etc.

Because users would be super confused if they have undertow extension (servlet) in their project but the lifecycle/behavior of the converstation context is completetly different (compared to the well-known built-in impl).

The lifecycle of the conversation context is defined by section 6.7.4-6.7.5 of the CDI spec. It is controlled by explicit user invocation of the Conversation interface. That section of the spec does not mention, and has never mentioned, anything about HTTP.

Section 11.3.4 outlines a particular implementation of a conversation context which is compatible with the Java EE web layer. That section of the spec is not supposed to be read as defining the behavior of the conversation context in general.

I'm talking about "conversation propagation" which is inherently implementation-specific and is in my opinion part of the "lifecycle definition", in case of the built-in impl there are cid and conversationPropagation request parameters.

And in my opinion it should not be based on a web technology because then it would not be usable outside an HTTP request anyway (which is the case for the built-in impl).

Indeed it should not be limited on HTTP. That's implicit, I guess.

:+1: but I'm not so sure everyone here takes it as implicit...

That said, as a matter of first impression, I don't see why it should not be doable in principle.

I'm not saying it's not doable but I do believe that we should start with a clean slate. To avoid confusion - I think that most of the users out there perceive the conversation context as something connected to an HttpSession. To avoid the need for built-in context impl (servlet based). To build an extendable AI-specific API.

@mkouba @ConversationScoped is an abstract notion, just like @RequestScoped. Its definition is completely clean of everything to do with HTTP. Just like you can have a request context for a request that does not originate in HTTP, you can have a conversation that doesn't take place over HTTP.

I really do understand all these concepts..

maxandersen commented 9 months ago

@mkouba all those issues about conversation propagation is something we would have with any kind of scope for this would it not ?

I don't think users sees @ConversationScope as something tied to http - I fully get why you feel it that way because its the variant having to be implemented.

But I would say we should not dismiss that @ConversationScope makes sense to (re)use just because of current Jakarta EE conversation scope implementation details.

gavinking commented 9 months ago

No matter what was intended I've never seen a custom conversation context impl. Hence it's a theory from my point of view.

OK, no problem, now you're (potentially) seeing it for the first time.

First of all, the built-in context (servlet-based) should be supported if you provide a custom one.

Naturally, but how's this an "obstacle"?

We deliberately chose not to implement this context

OK, I see. Then let me clarify my earlier response: naturally, if we already have HTTP session-based conversations then this new functionality would have to live peacefully side-by-side with the functionality that already exists. On the other hand, if no such functionality exists nor is needed, then there's no need to add it just because we have a different kind of conversation context.

We only need the things we need.

I'm talking about "conversation propagation" which is inherently implementation-specific

And quite deliberately so, given that a conversation is an abstract notion.

I'm not saying it's not doable but I do believe that we should start with a clean slate.

We would be talking about reusing one annotation @ConversationScoped and one interface Conversation. That's spitting distance from a "clean slate".

Now, it might indeed turn out that the Conversation interface is inappropriate in some respect, but you haven't demonstrated that yet. We would need to get a lot deeper into the weeds to be able to make that determination, it seems to me. But perhaps you've noticed something I've missed?

I think that most of the users out there perceive the conversation context as something connected to an HttpSession.

And now their perception will change, when they encounter the first instance of a conversation which is not connected to a HTTP session.

To avoid the need for built-in context impl (servlet based). To build an extendable AI-specific API.

That's what's being proposed, as far as I can tell. Nobody is saying that this should be based on servlets. As far as I can tell, the proposal is saying something quite different.

geoand commented 9 months ago

That's what's being proposed, as far as I can tell. Nobody is saying that this should be based on servlets. As far as I can tell, the proposal is saying something quite different.

Exactly

maxandersen commented 9 months ago

@mkouba @ConversationScoped is an abstract notion, just like @RequestScoped. Its definition is completely clean of everything to do with HTTP. Just like you can have a request context for a request that does not originate in HTTP, you can have a conversation that doesn't take place over HTTP.

similar issue arose about requestscope in reactive messaging - https://github.com/quarkusio/quarkus/issues/21367 here for long time it was not well-defined what @Requestscope means - in the end we ended up with that requestscope is not activated by default as its lifecycle would add overhead; but its possible to enable with @ActivateRequestContext

Wonder if other similar adaption would make sense.

gavinking commented 9 months ago

in reactive messaging - https://github.com/quarkusio/quarkus/issues/21367 here for long time it was not well-defined what @RequestScope means

See, I mean, I would have said (and I'm pretty sure I have said it plenty of times) that there's a perfectly clear and natural interpretation of @RequestScoped for any messaging service / event processing architecture. When the program receives a message, or is notified of an event, then processing of that particular message or event takes place within a request context that's tied to that message or event.

And one can even imagine a conversation context in this sort of environment: a context that's tied to the larger business process that encompasses a chain of messages/events. Indeed, we once had a conversation context just like that in JBPM. (Remember JBPM?!) This isn't so far removed from what we're talking about here, in fact.

in the end we ended up with that requestscope is not activated by default as its lifecycle would add overhead

This is really quite surprising to me. I mean, how would lazily creating a HashMap the first time a program explicitly injects a @RequestScoped object add significant overhead to requests which aren't making use of injection? Like ... where is the "overhead" coming from?

maxandersen commented 9 months ago

I think the english language gets in the way here and there are multiple "conversations" in play here.

Let me try and explain how I see the various "conversations" - tell me where I'm wrong/missing things :)

The "conversation" in LLM usecase are literally just a list of messages that contain the state that is growing over time as you have a "discussion" with the LLM. if you use chat.openai.com it matches to the list of previous "chats" on the left side in its UI. These conversations are data that can be in memory or even serialized out to disk/database. So those there will be multiple off per user.

While these "chat/discussion/conversation" are in the process memory you want to be able to set some trimming/eviction rules on them - at least not have them grow unbounded while in memory. Again, a user could have multiple of these.

Then there is the "conversation scope" which is a scope used to keep things around longer for a specific users interaction - and this is the kind of scope that (to me at least) makes sense to store those in-memory conversations. And for a users interaction I would say there would be just one conversation scope which potentially could have multiple LLM conversations.

The reason @ConversationScope makes sense to me is that it just fits nicely it seems:

A conversation represents a task—a unit of work from the point of view of the user. The conversation context holds state associated with what the user is currently working on. If the user is doing multiple things at the same time, there are multiple conversations.

The conversation context is active during any servlet request (since CDI 1.1). Most conversations are destroyed at the end of the request. If a conversation should hold state across multiple requests, it must be explicitly promoted to a long-running conversation.
gavinking commented 9 months ago

Look, I found the docs for JBPM-managed conversations in Seam: https://docs.jboss.org/seam/2.3.1.Final/reference/html/jbpm.html

maxandersen commented 9 months ago

in the end we ended up with that requestscope is not activated by default as its lifecycle would add overhead

This is really quite surprising to me. I mean, how would lazily creating a HashMap the first time a program explicitly injects a @RequestScoped object add significant overhead to requests which aren't making use of injection? Like ... where is the "overhead" coming from?

really need to hear from @cescoffier and @mkouba on that one. My understanding was that the boundaries was not clear when it should happen + context propagation as consequences became excessively expensive.

gavinking commented 9 months ago

My understanding was that the boundaries was not clear when it should happen + context propagation as consequences became excessively expensive.

So, I mean at a guess there's two things that might have been going on there:

  1. propagating context with Mutiny has been very problematic in the past for reasons that we've already discussed to death, and don't need to get into here, and
  2. I have the impression that previously people were thinking "too big" in terms of the lifecycle of a request context. Request contexts should be fine-grained. When people start saying that the boundaries of the request are unclear, I suspect them of trying to make the request context too big.

Indeed—and now I'm really just speculating—perhaps something that's been going on here is that people have been trying to propagate request contexts too far precisely because they felt they didn't have anything "bigger" to work with. But now maybe we're recognizing that there actually is a "bigger" thing: @ConversationScoped, and it has explicit demarcation in code, which helps make the boundaries clear. Not sure that's quite right, but perhaps it's a helpful lens.

cescoffier commented 9 months ago

a user may start multiple conversations concurrently, making things slightly more complicated.

@cescoffier so the wrinkle here is: can the "user" start multiple conversations from within the same request context? If so, then there's indeed some sort of mismatch with how @ConversationScoped is currently conceptualized. But if you can only start a single conversation within a given request context, I think it's a good fit. And, FTR, I don't know how "multiple conversations per request context" would even work. Seems like something that would be quite hard to fit into the whole model of CDI. (Note that "request context" ≠ HTTP request, you could of course do something to fork off a new request context, and start a conversation from there.)

Yes, in theory you can start multiple conversation concurrently from the same request scoped. Typically, the processing of a single HTTP request can start two conversation with 2 different AI services or even with the same. I don't think we should have a limitation here.

cescoffier commented 9 months ago

See, I mean, I would have said (and I'm pretty sure I have said it plenty of times) that there's a perfectly clear and natural interpretation of @RequestScoped for any messaging service / event processing architecture. When the program receives a message, or is notified of an event, then processing of that particular message or event takes place within a request context that's tied to that message or event.

Except that we have fork and join and things are super funky in these cases. At the moment, you can have the request scope for each of your processing steps, but there is no "one" propagated for the whole processing (multiple steps).

cescoffier commented 9 months ago

really need to hear from @cescoffier and @mkouba on that one. My understanding was that the boundaries was not clear when it should happen + context propagation as consequences became excessively expensive.

the overhead comes from the propagation of that context and the locks to handle concurrent access to that context.

gavinking commented 9 months ago

Yes, in theory you can start multiple conversation concurrently from the same request scoped.

Well sure, you can start multiple conversations from a single request, but they would each have their own request context.

We can't have multiple threads/reactive streams sharing a single request context, for reasons we've already discussed to death.

Except that we have fork and join and things are super funky in these cases.

Right, and that's exactly what I'm saying. There shouldn't ever be anything "funky" here, because whenever you fork the stream, that's a new request context.

This is a direct consequence of the axiom that the request context is single-threaded. (Or "single-streamed" if you prefer—exact same shit, different name.)

At the moment, you can have the request scope for each of your processing steps, but there is no "one" propagated for the whole processing (multiple steps).

Not sure what a "step" here is, but the way it should work is that a Mutiny stream is the analog of a thread, and a request context is propagated along the stream except when the stream forks, which is the exact analog of forking a thread.

I mean, we've already discussed this to death and we already know this is perfectly well-defined and completely correct.

the overhead comes from the propagation of that context

I mean there should in principle be zero overhead associated with propagating a request context. The issue was that Mutiny had no analog of a thread id. Once Mutiny has a well-defined thread id in each forked stream, there should simply be no overhead to speak of.

cescoffier commented 9 months ago

It's unrelated to Mutiny. A multi-step processing (multiple chained processing steps (each being a method)) may introduce many thread jumps - and every time context propagation kicks in. We made something more efficient with the duplicated context (which is preserved along the chain) - that's more or less our "thread id".

gavinking commented 9 months ago

A multi-step processing (multiple chained processing steps (each being a method)) may introduce many thread jumps and every time context propagation kicks in

I guess you're talking about the SmallRye Context Propagation stuff, which as you know I think is the wrong way to handle CDI context propagation, and I thought we had already agreed to move away from that. (It's reasonable for stuff like security context, etc.)

We made something more efficient with the duplicated context (which is preserved along the chain) - that's more or less our "thread id".

So in the past the issue was that forking a Mutiny thread would not result in a new duplicated context, so we still had the problem of forked Mutiny streams sharing a duplicated context, which was unsafe. I hope that's fixed now.

cescoffier commented 9 months ago

So in the past the issue was that forking a Mutiny thread would not result in a new duplicated context, so we still had the problem of forked Mutiny streams sharing a duplicated context, which was unsafe. I hope that's fixed now.

We are getting there. Soonish(tm). May happen this year or very beginning of next year.

manovotn commented 9 months ago

I don't think users sees @ConversationScope as something tied to http - I fully get why you feel it that way because its the variant having to be implemented.

See, I mean, I would have said (and I'm pretty sure I have said it plenty of times) that there's a perfectly clear and natural interpretation of @RequestScoped for any messaging service / event processing architecture. When the program receives a message, or is notified of an event, then processing of that particular message or event takes place within a request context that's tied to that message or event.

Sorry but I feel the need to chime in here. To this day, many (most?) CDI users perceive @RequestScoped as a scope tied to HTTP request. In this regard I am speaking from my own experience working on the spec, on Weld, monitoring SO questions etc. And that's despite the fact that specification explicitly supports other usages in several ways, one of which was mentioned on this issue - @ActivateRequestScope. There is no alternative use of conversation scope by the spec apart from the one tied to sessions these days plus I haven't seen a custom conversation context over the years either. So I'd say it's pretty safe to assume that users indeed will expect the @ConversationScoped to be tied to http.

So just because there is a theoretical possibility for different interpretation (and yes, there is[1]), it doesn't mean we need or should use that. Also note that the scope beings in some other luggage that we don't want - such as the fact that by definition, @ConversationScoped is a passivating[2] scope which is something we don't care about in Quarkus but according to the spec, this introduces additional restrictions and validations on any bean with that scope.

Say, what's blocking us from introducing a custom scope that will be tailored to whatever we intend to use it for? We can still use the original CDI's conversation scope as a parallel/comparison for users in our [java]docs but can design the API in whatever way we want/need with no strings attached to the original specification. Even if we end up with an API that's similar, I'd say it's better than bending over existing scope/context just because it's there and because you theoretically can. After all, CDI allows custom scopes for a reason :shrug:

[1] https://github.com/jakartaee/cdi/blob/main/api/src/main/java/jakarta/enterprise/context/ConversationScoped.java#L36-L38 [2] https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0#conversation_context_full

gavinking commented 9 months ago

We are getting there. Soonish(tm). May happen this year or very beginning of next year.

Excellent!

gavinking commented 9 months ago

To this day, many (most?) CDI users perceive @RequestScoped as a scope tied to HTTP request.

OK, but it's not that and it has never been that, so many (most?) CDI users are wrong.

And I'm not sure why we should care. Users are wrong about a great many things, and so we should not design our software to conform with the things that users are wrong about. Man, if only I had 49c for everything users are wrong about....

Say, what's blocking us from introducing a custom scope that will be tailored to whatever we intend to use it for?

Nothing is blocking us from doing that, and that might indeed be the best thing to do. I'm patiently waiting to see an actually good argument for why it would be better. And so far nobody has presented any such argument. To come up with an argument like that requires one to carefully think through the details.

Even if we end up with an API that's similar, I'd say it's better than bending over existing scope/context just because it's there and because you theoretically can.

I would say it's almost always a mistake to introduce a new nonstandard API that's isomorphic to an existing standard API.

But perhaps there really are some concrete differences there. We should try to identify them!

manovotn commented 9 months ago

And I'm not sure why we should care.

Because you want users to understand the API they are about to use as easily as possible?

Users are wrong about a great many things, and so we should not design our software to conform with the things that users are wrong about. Man, if only I had 49c for everything users are wrong about....

I never indicated we should conform to wrong understanding because it is widely accepted. Instead I am saying we can side step any such confusion preemptively by simply using a custom scope.

Nothing is blocking us from doing that, and that might indeed be the best thing to do. I'm patiently waiting to see an actually good argument for why it would be better.

Works the other way around too. What's the actually good argument to reuse conversation scoped as opposed to custom scope? Is it the familiarity of the scope which is also mislinked to one specific usage? Or that some of the API methods seem like what we would use? What if we want to evolve the API or change how some of the methods behave which won't sit with the current spec wording?

Also note the bit I wrote about conversation scope being passivating=true which then implies certain spec behavior such as this - https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0#passivation_validation I don't think we want to deal with anything linked to passivation in Quarkus and so picking the scope and choosing to ignore some of the specific that just don't happen to fit us seems like a poor choice. I am well aware of the fact that CDI Lite, which we implement, doesn't have conversation scope in it, but it's IMO still a poor excuse for reusing the context and ignoring others parts that go hand in hand with it in the spec (and would be tested by TCKs).

I would say it's almost always a mistake to introduce a new nonstandard API that's isomorphic to an existing standard API.

Yeah, I don't know the exact ins and outs of this usage and how it can potentially differ or grow in the future. But a custom scope can have well-defined minimal behavior specific to the usage we intend to leverage it for. That alone should differentiate the APIs.

maxandersen commented 9 months ago

What I, and I'll dare say @gavinking, is saying is let's not discard the idea of reusing @conversationscope just because of some perceived baggage. And we also say let's not pick conversationscope just because we say so.

The conversation here revealed things to figure out which is relevant with and without conversationscope.

Let's call what we are trying to find for @wonkascope and see what it looks like - if it ends up having same semantics as conversationscope we don't have to introduce another one. If it's different - we then are able to explain why it's not same as conversation scope.

But when we have it (de)mar cation and secmantics of its behavior still need to be explainable/fitting into the rest.

gavinking commented 9 months ago

Instead I am saying we can side step any such confusion preemptively by simply using a custom scope.

But you didn't actually give us any reason to think that this will confuse users, once it exists. All you did was claim, without evidence, that users hold a certain belief today. That doesn't show that users are incapable of updating their beliefs in the future, when presented with a good reason to do so.

The "confusion" you claim that users will experience upon encountering this new functionality in the future is entirely speculative, and I feel quite entitled to deny that there's any good reason why it should be a common experience. There's nothing really objectively confusing here. Software developers are quite accustomed to the notion that a single API might abstract different implementations.

I mean, you don't seem to be confused by what's being proposed. You're merely speculating that our users are stupider than we are, and will therefore be confused by a thing we don't ourselves find confusing. The premise might be true—though I think I could dig up plenty of evidence which would tend to throw the premise in doubt—but the inference is unsound.

What's the actually good argument to reuse conversation scoped as opposed to custom scope?

Because it already exists and seems to have the semantics we require. And one should not introduce new API constructs when existing constructs already capture what's needed. Occam's razor. That's all the justification I need.

Also note the bit I wrote about conversation scope being passivating=true which then implies certain spec behavior such as this

I'm sorry, but I'm just struggling to see how this is a real objection at all. I'm pretty sure you could come up with three solutions to this problem in less time than it took you to find that link to the spec.

Yeah, I don't know the exact ins and outs of this usage and how it can potentially differ or grow in the future. But a custom scope can have well-defined minimal behavior specific to the usage we intend to leverage it for. That alone should differentiate the APIs.

Sorry for being blunt man, but that's just a bunch of handwaving. If you think these things might be different, sit down and figure out what the concrete points of difference are.

gavinking commented 9 months ago

Let's call what we are trying to find for @wonkascope and see what it looks like - if it ends up having same semantics as conversationscope we don't have to introduce another one. If it's different - we then are able to explain why it's not same as conversation scope.

This 100%.

Figure out what the semantics are, what the implementation is sorta going to look like, and exactly what points of control we need to expose to the client program. And then decide whether this is a new scope, or one which fits a model we already have.

agoncal commented 9 months ago

What's the actually good argument to reuse conversation scoped as opposed to custom scope?

If naming things is hard, when you use a chatbot, you enter into a "conversation" between the user and the assistant. So ConversationScoped is actually a great name. Then, if it's not doable to reuse the existing annotation, fine, but in terms of semantics, it's a good start.