feat[ai]: Add conversation endpoints. #1132

chirino · 2025-01-10T21:00:11Z

LLM internal state messages is simplified to be a single field either on ChatState or in the Conversation table. We now keep timestamps for individual ChatMessages.

Also drop the db connection arg from the completions method as the DB is Not needed for those calls.

You can test out the completions API using the UI at: https://github.com/chirino/ai-assistant-vite

just npm i && npm run dev and use the browser link outputted.

ctron · 2025-01-13T08:43:41Z

modules/fundamental/src/ai/endpoints/mod.rs

@@ -1,16 +1,22 @@
 #[cfg(test)]
 mod test;

+use crate::ai::model::{Conversation, ConversationSummary};


I would be great if you could group the use statements.

If this preference is important this should be enforced by the xtask precommit command, otherwise it will not be uniformly enforced.

Can you add that then?

I personally don't mind it being ungrouped, my IDE automatically adds them that way. I also don't know how to get the format checker to enforce that.

Looks like https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#imports_granularity is what we would want. but it's only in unstable right now.

Why not keep it simple and just manually do it? IntelliJ (Rust Rover) helps with "nest use statements":

If we can't automatically fix it for them /w cargo fmt, we should not be nitpicking about it in PR reviews. This is all just stylistic right?

ctron

Maybe a few things to understand or fix.

ctron · 2025-01-13T08:47:01Z

modules/fundamental/src/ai/endpoints/mod.rs

+    responses(
+        (status = 200, description = "The resulting conversation", body = Conversation),
+        (status = 400, description = "The request was invalid"),
+        (status = 404, description = "The AI service is not enabled or the conversation was not found")


It looks to me like there's an inconsistency in generating this. When the conversion is not found, it returns "ok, but emtpy" if the conversion is found, but doesn't belong to the user, it returns "not found".

Yeah. This is because we fake the create_conversation. All it does is generate an empty conversation with a generated UUID. This is because the UI calls create_conversation when the chat page is loaded. If we stored the empty conversation at this point, we would end up with lots of empty/abandoned conversations in the DB.

We could require create_conversation to at least contain a message, but this would make the API a little clunkier to use from the UI.

Ok, but in that case the part of "or the conversation was not found" is not true, is it?

well yes. It actually means that the conversation belongs to another user, but I'd rather lie than tell them that.

But you're leaking that information anyway. Every time you get back a 404, it means "not allowed", or "other other".

ctron · 2025-01-13T08:51:05Z

modules/fundamental/src/ai/service/mod.rs

    }

-    pub async fn update_conversation<C: ConnectionTrait>(
+    pub async fn upsert_conversation<C: ConnectionTrait>(


In the implementation is says that the update can take a while. How will that affect concurrent calls this this function. Which to my understanding can happen when multiple calls via the API happen.

This is true. The API caller is responsible for increasing the seq arg in each subsequent/concurrent call. This should allow only the latest update win since we do a:

update(model) .filter(conversation::Column::Seq.lte(seq))

yes: If the caller want's to be lazy and leave it to chance, he can leave the seq number unchanged.

We should probably insert the record before LLM call to avoid a race on insert.

Ah, the seq is basically the "oplock", is it?

If that's the case, why not follow the pattern that we already have:

trustify/modules/importer/src/endpoints.rs

Line 99 in 26e33cb

.append_header((header::ETAG, ETag(EntityTag::new_strong(revision))))

trustify/modules/importer/src/endpoints.rs

Line 110 in 26e33cb

("if-match"=Option<String>, Header, description = "The revision to update"),

but given seq is part of the data model response object, seems odd to post it via headers.

The pattern of If-Match and ETag seems to be a standard thing.

I think that tend to be a thing when the etag is not part of the document. Example: hash of a file etc.

But I guess it can't hurt to use the etag system. It should help with caching too.

Why invent something new, if there's an existing pattern?!

ctron · 2025-01-15T07:41:22Z

modules/fundamental/src/ai/endpoints/mod.rs

 use itertools::Itertools;
+use time::OffsetDateTime;
+use trustify_auth::authenticator::user::UserDetails;


I'd still prefer to have this nested. Or at least, consistent.

chirino · 2025-01-15T15:25:44Z

will squash and rebase..

LLM internal state messages is simplified to be a single field either on ChatState or in the Conversation table. We now keep timestamps for individual ChatMessages. Also drop the db connection arg from the completions method as the DB is Not needed for those calls. Signed-off-by: Hiram Chirino <[email protected]>

chirino force-pushed the conversations-endpoints branch from 773e6d5 to 0585e3c Compare January 10, 2025 23:31

chirino requested a review from mrizzi January 11, 2025 15:19

ctron reviewed Jan 13, 2025

View reviewed changes

ctron requested changes Jan 13, 2025

View reviewed changes

ctron approved these changes Jan 15, 2025

View reviewed changes

chirino force-pushed the conversations-endpoints branch from d8e2a65 to 5c5ed5b Compare January 15, 2025 15:35

chirino added this pull request to the merge queue Jan 15, 2025

Merged via the queue into trustification:main with commit 88424ad Jan 15, 2025
1 check passed

chirino deleted the conversations-endpoints branch January 15, 2025 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat[ai]: Add conversation endpoints. #1132

feat[ai]: Add conversation endpoints. #1132

chirino commented Jan 10, 2025 •

edited

Loading

ctron Jan 13, 2025

chirino Jan 13, 2025

ctron Jan 13, 2025

chirino Jan 13, 2025

chirino Jan 13, 2025

ctron Jan 14, 2025

chirino Jan 14, 2025

ctron left a comment

ctron Jan 13, 2025

chirino Jan 13, 2025

ctron Jan 13, 2025

chirino Jan 13, 2025

ctron Jan 14, 2025

ctron Jan 13, 2025

chirino Jan 13, 2025 •

edited

Loading

ctron Jan 13, 2025 •

edited

Loading

chirino Jan 13, 2025

chirino Jan 13, 2025

ctron Jan 14, 2025

chirino Jan 14, 2025

chirino Jan 14, 2025

ctron Jan 15, 2025

ctron Jan 15, 2025

chirino commented Jan 15, 2025

feat[ai]: Add conversation endpoints. #1132

feat[ai]: Add conversation endpoints. #1132

Conversation

chirino commented Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chirino Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

ctron Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chirino commented Jan 15, 2025

chirino commented Jan 10, 2025 •

edited

Loading

chirino Jan 13, 2025 •

edited

Loading

ctron Jan 13, 2025 •

edited

Loading