-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: aila categoriser feature with chat ID and user ID #12
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Playwright e2e testsTo view traces locally, unzip the report and run: npx playwright show-report ~/Downloads/playwright-report |
@@ -65,7 +65,7 @@ export class LessonPlans { | |||
private _prisma: PrismaClientWithAccelerate; | |||
constructor(private readonly prisma: PrismaClientWithAccelerate) { | |||
this._prisma = prisma; | |||
this._rag = new RAG(this._prisma); | |||
this._rag = new RAG(this._prisma, { chatId: "none" }); |
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.
Models are mainly used for ingest, so we don't have a chatId or a userId
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 wondered about having a special string for this. We might want to do same for userId at some point? Perhaps not
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.
Yes we might want to also store something for the user ID in non-user calls. I'm expecting we look through the database at some point to see where we are missing what we need, and that "none" should mean we can see those rows. I had to pass a string here because the underlying method now throws if chatId is not supplied.
Quality Gate passedIssues Measures |
@@ -285,4 +286,37 @@ describe("Aila", () => { | |||
expect(ailaInstance.lesson.plan.title).toBe(newTitle); | |||
}, 20000); | |||
}); | |||
|
|||
describe("categorisation", () => { |
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.
Just adding one example test here to show how we can pass in the mock categoriser during Aila initialisation
@@ -23,6 +23,7 @@ export interface AilaLessonService { | |||
readonly hasSetInitialState: boolean; | |||
applyPatches(patches: string): void; | |||
initialise(plan: LooseLessonPlan): void; | |||
setUpInitialLessonPlan(messages: Message[]): Promise<void>; |
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.
Moves the initial lesson plan setup to the Aila Lesson which makes more sense - resolves the TODO in the Aila class
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.
Looks good, couple queries
this._prisma = options.prisma ?? globalPrisma; | ||
|
||
this._lesson = new AilaLesson({ | ||
aila: this, |
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.
Have seen some fiddly bugs arise from passing aila: this
as a prop, as the types aren't necessarily accurate (due to a quasi race condition on instantiation)
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.
Not saying don't do this, just highlighting because I think you were away when it was discovered
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.
Okay that's interesting! I see. So if you've not yet set some property on the instance before you pass it, then it could cause a problem.
@@ -65,7 +65,7 @@ export class LessonPlans { | |||
private _prisma: PrismaClientWithAccelerate; | |||
constructor(private readonly prisma: PrismaClientWithAccelerate) { | |||
this._prisma = prisma; | |||
this._rag = new RAG(this._prisma); | |||
this._rag = new RAG(this._prisma, { chatId: "none" }); |
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 wondered about having a special string for this. We might want to do same for userId at some point? Perhaps not
🎉 This PR is included in version 1.3.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description