-
Notifications
You must be signed in to change notification settings - Fork 1
Implement MVP podcast generation #34
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
base: dev
Are you sure you want to change the base?
Conversation
backend/app/api/routes/podcasts.py
Outdated
|
|
||
| @router.get("/{course_id}", response_model=PodcastsPublic) | ||
| def list_podcasts(course_id: uuid.UUID, session: SessionDep, current_user: CurrentUser) -> Any: | ||
| pods = session.exec(select(Podcast).where(Podcast.course_id == course_id)).all() |
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.
Some pagination might be useful here
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 can add this as a feature in another PR on both the backend and frontend.
michaelgichia
left a comment
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.
This is really impressive, and I will be testing it out in a few hours. I've left a few comments before it is merge.
|
|
||
| const baseURL = | ||
| process.env.NEXT_PUBLIC_BACKEND_BASE_URL ?? 'http://localhost:8000' | ||
| const baseURL = isServer |
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.
You are probably running into this error because you are not using the client.
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 can remove this in this PR but this was an issue I ran into even before implementing podcast. I couldn't log in or sign up. (PS: I'm running the project with docker)
backend/app/api/routes/podcasts.py
Outdated
| teacher_voice = body.teacher_voice if body and body.teacher_voice else settings.PODCAST_TEACHER_VOICE | ||
| student_voice = body.student_voice if body and body.student_voice else settings.PODCAST_STUDENT_VOICE | ||
| narrator_voice = body.narrator_voice if body and body.narrator_voice else settings.PODCAST_TEACHER_VOICE |
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.
All these aren't really necessary if we use enum type and specify a default value
deluakin
left a comment
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.
Good start but some improvement will be needed
| * Total Submitted | ||
| */ | ||
| total_submitted: number; | ||
| /** | ||
| * Total Correct | ||
| */ | ||
| total_correct: number; | ||
| /** | ||
| * Score Percentage |
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.
FYI: the deletions in the autogenerated files happened automatically after I ran the client generation script #34 (comment)
|
@deluakin @michaelgichia Thanks for your review! |
michaelgichia
left a comment
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.
A few comments, and this should be good. Testing now.
| "id": embedding_uuid, | ||
| "values": embedding, | ||
| "metadata": { | ||
| "course_id": str(document.course_id), |
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.
Thanks for fixing this. I was curious why it wasn't working.
backend/app/api/routes/podcasts.py
Outdated
|
|
||
|
|
||
| @router.get("/course/{course_id}", response_model=PodcastsPublic) | ||
| def list_podcasts(course_id: uuid.UUID, session: SessionDep, _current_user: CurrentUser) -> PodcastsPublic: |
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.
| def list_podcasts(course_id: uuid.UUID, session: SessionDep, _current_user: CurrentUser) -> PodcastsPublic: | |
| def list_podcasts(course_id: uuid.UUID, session: SessionDep, _current_user: CurrentUser, skip: int = 0, limit: int = 50) -> PodcastsPublic: |
| } | ||
|
|
||
| export const config = { | ||
| runtime: 'nodejs', |
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.
| runtime: 'nodejs', | |
| runtime: 'nodejs', | |
| maxDuration: 300, |
| console.error('[PodcastAudio] Audio stream error:', error) | ||
| } | ||
|
|
||
| return Response.json({ error: 'Failed to stream audio' }, { status: 500 }) |
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.
Return the error like this, otherwise, the client will fail because it expects a specific payload structure:
const status: number = get(
error as Record<string, never>,
'response.status',
500,
)
const body: ErrorResponse = get(
error as Record<string, never>,
'response.data.detail',
{
detail: 'Internal Server Error',
},
)
return NextResponse.json(body, {status})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 figured it didn't matter because I wasn't using the ErrorBox component, which relies on error.detail, but I can add in case of future use
| import {CourseWithDocuments} from '@/client' | ||
| import FileCard from '@/components/ui/file-card' | ||
| import {getCourse} from '@/lib/courses' | ||
| import {getCourse} from '@/actions/courses' |
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.
Why the change?
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.
for consistency, as I noticed you were importing getCourse directly from '@/actions/courses' in the other parts of the dashboard
|
|
||
| import {CourseWithDocuments} from '@/client' | ||
| import {getCourse} from '@/lib/courses' | ||
| import {getCourse} from '@/actions/courses' |
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.
Why the change?
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.
|
We point the PRs to dev first. |
| // Configure axios client per request | ||
| client.setConfig({ | ||
| baseURL: process.env.NEXT_PUBLIC_BACKEND_BASE_URL as string, | ||
| baseURL: process.env.NEXT_INTERNAL_BACKEND_BASE_URL as string, |
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.
@michaelgichia After merging to dev, I noticed this was changed to NEXT_PUBLIC_BACKEND_BASE_URL but I had to return it to NEXT_INTERNAL_BACKEND_BASE_URL for things to run on my local.
Not sure if this is fine.
Summary
Adds an MVP “Podcast” feature with local/S3 storage, a custom player, and RAG‑driven content.
What’s Included
course_idand optionaldocument_idsHow To Use
Config
.envPODCAST_STORAGE=local|s3PODCAST_LOCAL_DIR=/app/podcastsS3_BUCKET_NAME,AWS_REGION, credentialsPODCAST_TEACHER_VOICE,PODCAST_STUDENT_VOICEVerification
Screenshot
Unrelated Podcast Changes (and Why)
These changes are not directly part of the podcast feature but were needed to get the app running on my local machine, including some debugging logs to know the flow of data as I also had issues getting chat to work.
alembic upgrade.course_id), added diagnostics; later leveraged by podcast RAG.ChatPublicschema to avoid validation errors when returning chat history.suppressHydrationWarningon<html>for next-themes. I think it is safe here becuase client toggles the theme class and SSR defaults match, so we’re not hiding real mismatches.