ai-transport: add message per response doc #3018
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zknill
force-pushed
the
zak/message-per-response
branch
from
9c6215e to
bb19f32
Compare
GregHolmes
added
review-app
GregHolmes
left a comment
GregHolmes
left a comment
There was a problem hiding this comment.
I've received from a docs perspective, just a couple comments. Happy to discuss if needed.
src/pages/docs/ai-transport/features/token-streaming/message-per-response.mdx
Outdated
Show resolved
Hide resolved
src/pages/docs/ai-transport/features/token-streaming/message-per-response.mdx
Outdated
Show resolved
Hide resolved
src/pages/docs/ai-transport/features/token-streaming/message-per-response.mdx
Outdated
Show resolved
Hide resolved
src/pages/docs/ai-transport/features/token-streaming/message-per-response.mdx
Outdated
Show resolved
Hide resolved
zknill
force-pushed
the
zak/message-per-response
branch
from
65e1836 to
87dd2a2
Compare
GregHolmes
force-pushed
the
AIT-129-AIT-Docs-release-branch
branch
from
8c010c8 to
5014fd3
Compare
GregHolmes
left a comment
GregHolmes
left a comment
There was a problem hiding this comment.
Happy with this from a docs perspective.
GregHolmes
force-pushed
the
zak/message-per-response
branch
from
87dd2a2 to
0e4d434
Compare
GregHolmes
force-pushed
the
AIT-129-AIT-Docs-release-branch
branch
2 times, most recently
from
a863e09 to
2f792d4
Compare
|
|
||
| This pattern allows publishing append operations for multiple concurrent model responses on the same | ||
| channel. As long as you append to the correct message serial, tokens from different responses will | ||
| not interfere with each other. And the final concatenated message for each response will contain only the tokens |
There was a problem hiding this comment.
| not interfere with each other. And the final concatenated message for each response will contain only the tokens | |
| not interfere with each other, and the final concatenated message for each response will contain only the tokens |
|
|
||
| <Code> | ||
| ```javascript | ||
| const realtime = new Ably.Realtime('YOUR_API_KEY'); |
There was a problem hiding this comment.
We should use {{API_KEY}} which will interpolate a value
|
|
||
| ## Requirements and restrictions | ||
|
|
||
| ### Channel rule |
There was a problem hiding this comment.
I think this should appear earlier in the page, as the user has no indication that they need to configure the rule to use the code examples above until this point. Also the code examples should have a namespace prefix in the channel name
|
|
||
| const { serials: [msgSerial] } = await channel.publish('ai-response', { data: '' }); | ||
|
|
||
| for await (const token of streamedResponse) { |
There was a problem hiding this comment.
I think we should include a comment indicating what this example assumes is returned from the stream, like we do in https://github.com/ably/docs/pull/3014/changes#diff-b66bbc76507fb59b061f92037a82a9884804c3668f11a277fb5495b6a3819deb
Also nit, but we should use a consistent variable name for the stream, it is called stream elsewhere
| not interfere with each other. And the final concatenated message for each response will contain only the tokens | ||
| from that response. | ||
|
|
||
| ### Complete publish example |
There was a problem hiding this comment.
After this header we should have some preamble text describing what the example shows
| case 'message.append': | ||
| console.log('Token received:', msg.data); | ||
| // Append the token to your UI | ||
| appendToResponse(msg.data); |
There was a problem hiding this comment.
I tried to avoid using this kind of pattern in the other docs: https://github.com/ably/docs/pull/3014/changes#diff-b66bbc76507fb59b061f92037a82a9884804c3668f11a277fb5495b6a3819deb
I'm not sure what the recommended approach is, but I'd rather include examples that are self contained and how some kind of simple handling of the results.
So I think in this case, it would make sense to have a Map with messages stored by serial, and we show how to add a value to the map on create (or replace it on update) and concatenate tokens to it on append. Then remove the imaginary displayNewResponse, appendToResponse and replaceResponse functions.
| concatenated response. | ||
|
|
||
| Occasionally you may receive a `message.update` action, which indicates that the channel needs to | ||
| stream the entire message data so far. This can happen if a client has been offline for a while, and |
There was a problem hiding this comment.
This can happen if a client has been offline for a while
I think we should be more precise here, e.g.:
For example, this can happen if the client resumes after a transient disconnection and the channel ...
| case 'message.create': | ||
| console.log('New response started:', msg.data); | ||
| // A new response has started | ||
| displayNewResponse(msg.data); |
There was a problem hiding this comment.
Same comment as above
| from the point of attachment backward: | ||
|
|
||
| <Code> | ||
| ```javascript |
There was a problem hiding this comment.
Can we align this code example with the one used in #3014 ? They shouldn't be too different and I think good to document a consistent approach
| </Code> | ||
|
|
||
|
|
||
| ### Using history with untilAttach |
There was a problem hiding this comment.
Can we add a pattern for aligning with complete responses loaded from your database, like we do in #3014 ?
GregHolmes
force-pushed
the
zak/message-per-response
branch
from
0e4d434 to
e1dd9a8
Compare
mschristensen
force-pushed
the
zak/message-per-response
branch
from
a1a1183 to
7ef1e17
Compare
Add doc explaining streaming tokens with appendMessage and update compaction allowing message-per-response history.
Unifies the token streaming nav for token streaming after rebase.
Refines the intro copy in message-per-response to have structural similarity with the message-per-token page.
Refine the Publishing section of the message-per-response docs. - Include anchor tags on title - Describe the `serial` identifier - Align with stream pattern used in message-per-token docs - Remove duplicate example
Refine the Subscribing section of the message-per-response docs. - Add anchor tag to heading - Describes each action upfront - Uses RANDOM_CHANNEL_NAME
Refine the rewind section of the message-per-response docs. - Include description of allowed rewind paameters - Tweak copy
Refines the history section for the message-per-response docs. - Adds anchor to heading - Uses RANDOM_CHANNEL_NAME - Use message serial in code snippet instead of ID - Tweaks copy
Fix the hydration of in progress responses via rewind by using the responseId in the extras to correlate messages with completed responses loaded from the database.
Fix the hydration of in progress responses using history by obtaining the timestamp of the last completed response loaded from the database and paginating history forwards from that point.
Removes the headers/metadata section, as this covers the specific semantics of extras.headers handling with appends, which is better addressed by the (upcoming) message append pub/sub docs. Instead, a callout is used to describe header mixin semantics in the appropriate place insofar as it relates to the discussion at hand.
Update the token streaming with message per token docs to include a callout describing resume behaviour in case of transient disconnection.
Fix the message per token docs headers to include anchors and align with naming in the message per response page.
mschristensen
force-pushed
the
zak/message-per-response
branch
from
7ef1e17 to
5438a58
Compare
Add doc explaining streaming tokens with appendMessage and update compaction allowing message-per-response history.