Skip to content
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: rag new schema, standalone package #448

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

mantagen
Copy link
Collaborator

@mantagen mantagen commented Dec 12, 2024

Description

BEHIND FEATURE FLAG // not ready for review

  • standalone rag packages
  • points to rag schema
  • new 'publish' step in ingest pacakge

Copy link

vercel bot commented Dec 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
oak-ai-lesson-assistant ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 9, 2025 6:23pm

Copy link

github-actions bot commented Dec 12, 2024

Playwright test results

passed  17 passed
flaky  1 flaky
skipped  2 skipped

Details

report  Open report ↗︎
stats  20 tests across 17 suites
duration  2 minutes, 32 seconds
commit  f83a6ae

Flaky tests

No persona › tests/modifiy-lesson.test.ts › Modify a lesson plan › Modify a lesson resource

Skipped tests

No persona › tests/auth.test.ts › authenticate through Clerk UI
No persona › tests/chat-performance.test.ts › Component renders during lesson chat › There are no unnecessary rerenders across left and right side of chat


id String @id @default(cuid())
oakLessonId Int? @map("oak_lesson_id")
oakLessonSlug String @map("oak_lesson_slug")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds slug as a mandatory field

lessonPlan Json @map("lesson_plan") @db.JsonB
subjectSlug String @map("subject_slug")
keyStageSlug String @map("key_stage_slug")
isPublished Boolean @default(false) @map("is_published")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds isPublished field

eyfs: "early-years-foundation-stage",
};

export function parseKeyStage(maybeKeyStage: string): string {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self - use parseKeyStage function from posthog event helpers

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for rag/parseKeyStage I imagine

Copy link
Collaborator

@codeincontext codeincontext left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good on the whole. There are some parts which could benefit from being extracted to functions, but it's not a big deal. For example when transforming data or making raw queries. This could:

  • Make conditional / loop logic to be simpler to follow and simpler for typescript
  • Make high level process clearer, as the lower level detail is tucked away and replaced with useful function names
  • Make unit testing simpler, and make it simpler for future devs to update a single part

This relates to code style and isn't a blocker

};

export function parseKeyStage(maybeKeyStage: string): string {
maybeKeyStage = maybeKeyStage.toLowerCase().replace(/[^a-z0-9]/g, "");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sonar might complain here about reassigning an argument

Comment on lines 38 to +42
lessonPlan = CompletedLessonPlanSchema.parse(
JSON.parse(maybeLessonPlanString),
);

lessonPlan.keyStage = parseKeyStage(lessonPlan.keyStage);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I personally have a preference for this style, but it's not important!

Suggested change
lessonPlan = CompletedLessonPlanSchema.parse(
JSON.parse(maybeLessonPlanString),
);
lessonPlan.keyStage = parseKeyStage(lessonPlan.keyStage);
const parsedLessonPlan = CompletedLessonPlanSchema.parse(
JSON.parse(maybeLessonPlanString),
);
lessonPlan = {
...parsedLessonPlan,
keyStage: parseKeyStage(lessonPlan.keyStage)
};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, shouldn't let lessonPlan have a type?

eyfs: "early-years-foundation-stage",
};

export function parseKeyStage(maybeKeyStage: string): string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for rag/parseKeyStage I imagine

Comment on lines +46 to +73
const ragLessonPlans: {
oakLessonId?: number;
oakLessonSlug: string;
ingestLessonId?: string;
subjectSlug: string;
keyStageSlug: string;
lessonPlan: LooseLessonPlan;
}[] = [];

for (const lesson of lessons) {
if (!lesson.lessonPlan) {
throw new IngestError("Lesson is missing lesson plan", {
ingestId,
lessonId: lesson.id,
});
}

const lessonPlan = LessonPlanSchema.parse(lesson.lessonPlan.data);

ragLessonPlans.push({
oakLessonId: lesson.oakLessonId,
oakLessonSlug: lesson.data.lessonSlug,
ingestLessonId: lesson.id,
subjectSlug: lesson.data.subjectSlug,
keyStageSlug: lesson.data.keyStageSlug,
lessonPlan,
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A functional approach for this (may or may not be better in your eyes!)

Suggested change
const ragLessonPlans: {
oakLessonId?: number;
oakLessonSlug: string;
ingestLessonId?: string;
subjectSlug: string;
keyStageSlug: string;
lessonPlan: LooseLessonPlan;
}[] = [];
for (const lesson of lessons) {
if (!lesson.lessonPlan) {
throw new IngestError("Lesson is missing lesson plan", {
ingestId,
lessonId: lesson.id,
});
}
const lessonPlan = LessonPlanSchema.parse(lesson.lessonPlan.data);
ragLessonPlans.push({
oakLessonId: lesson.oakLessonId,
oakLessonSlug: lesson.data.lessonSlug,
ingestLessonId: lesson.id,
subjectSlug: lesson.data.subjectSlug,
keyStageSlug: lesson.data.keyStageSlug,
lessonPlan,
});
}
const ragLessonPlans = lessons.map(lesson =>
if (!lesson.lessonPlan) {
throw new IngestError("Lesson is missing lesson plan", {
ingestId,
lessonId: lesson.id,
});
}
const lessonPlan = LessonPlanSchema.parse(lesson.lessonPlan.data);
return ({
oakLessonId: lesson.oakLessonId,
oakLessonSlug: lesson.data.lessonSlug,
ingestLessonId: lesson.id,
subjectSlug: lesson.data.subjectSlug,
keyStageSlug: lesson.data.keyStageSlug,
lessonPlan,
});
})

@@ -0,0 +1,97 @@
/**
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this file be index.test.ts?

Copy link

sonarqubecloud bot commented Jan 9, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants