Skip to content

feat: Clean up sample app and finalize endpoints #26

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.

Sign up for GitHub

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

Merged
shuangela merged 4 commits into from
Nov 6, 2025
Merged

feat: Clean up sample app and finalize endpoints #26

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a29569f
clean up sample app
shuangela Nov 5, 2025
bc6a612
add explanatory comment
shuangela Nov 5, 2025
2b71ff3
add new changes
shuangela Nov 6, 2025
07ca7f2
Merge branch 'development' into python-backend-cleanup
shuangela Nov 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions server/python/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from fastapi import FastAPI
from fastapi.middleware.cors import CORSMiddleware
from src.routers import movies
from src.utils.errorHandler import register_error_handlers
from src.utils.errorHandler import register_error_handlers, create_error_response
Copy link
Copy Markdown
Collaborator

@dacharyc dacharyc Nov 6, 2025

Choose a reason for hiding this comment

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

You're adding create_error_response here but it looks like it's not used. Are we missing code that was meant to use it, or was this added in error?

Suggested change
from src.utils.errorHandler import register_error_handlers, create_error_response
from src.utils.errorHandler import register_error_handlers

Copy link
Copy Markdown
Collaborator Author

@shuangela shuangela Nov 6, 2025

Choose a reason for hiding this comment

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

added in error, we only use it in models.py. i will remove it

from src.database.mongo_client import db, get_collection
import traceback
Copy link
Copy Markdown
Collaborator

@dacharyc dacharyc Nov 6, 2025

Choose a reason for hiding this comment

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

Looks like we're not using this import - maybe it's left over from a prior version of this file?

Suggested change
import traceback

import os
Expand Down Expand Up @@ -34,7 +34,6 @@ async def ensure_search_index():
indexes = [idx async for idx in result]
index_names = [index["name"] for index in indexes]
if "movieSearchIndex" in index_names:
print("MongoDB Search index already exists.")
return

# Create a mapping if the movieSearchIndex does not exist
Expand All @@ -58,9 +57,12 @@ async def ensure_search_index():
"definition": index_definition
}]
})
print("MongoDB Search index created.")
except Exception as e:
print(f"Error creating the search index: {e}")
raise RuntimeError(
f"Failed to create search index 'movieSearchIndex': {str(e)}. "
f"Search functionality may not work properly. "
f"Please check your MongoDB Atlas configuration and ensure the cluster supports search indexes."
)

@app.on_event("startup")
Copy link
Copy Markdown
Collaborator

@dacharyc dacharyc Nov 6, 2025

Choose a reason for hiding this comment

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

It looks like the on_event handler we're using here is deprecated. The docs say we should be using lifespan event handlers instead: https://fastapi.tiangolo.com/advanced/events/#alternative-events-deprecated

I don't know how big of a lift it is to switch to the lifespan event handler. If this isn't a quick fix, I'd say we should scope this for a fast follow given the release timeline. But ideally we would not release a brand new sample app that uses deprecated APIs.

Copy link
Copy Markdown
Collaborator Author

@shuangela shuangela Nov 6, 2025

Choose a reason for hiding this comment

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

i will look into this and try to implement it in my upcoming push!

async def vector_search_index():
Expand All @@ -69,7 +71,6 @@ async def vector_search_index():
This ensures the index is ready before any vector search requests are made.
"""
try:

embedded_movies_collection = get_collection("embedded_movies")

# Get list of existing indexes - convert AsyncCommandCursor to list
Expand Down Expand Up @@ -98,9 +99,12 @@ async def vector_search_index():

# Create the index
result = await embedded_movies_collection.create_search_index(index_definition)
Copy link
Copy Markdown
Collaborator

@dacharyc dacharyc Nov 6, 2025

Choose a reason for hiding this comment

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

Looks like we're not doing anything with this result, so we could omit it.

Suggested change
result = await embedded_movies_collection.create_search_index(index_definition)
await embedded_movies_collection.create_search_index(index_definition)

print("Vector search index 'vector_index' ready to query.")

except Exception as e:
print(f"Error during vector search index setup: {str(e)}")
print(f"Error type: {type(e).__name__}")
raise RuntimeError(
f"Failed to create vector search index 'vector_index': {str(e)}. "
f"Vector search functionality will not be available. "
f"Please check your MongoDB Atlas configuration, ensure the cluster supports vector search, "
f"and verify the 'embedded_movies' collection exists with the required embedding field."
)

Loading
⚡