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

chore: add timeout for api #1638

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nquang29
Copy link
Collaborator

@nquang29 nquang29 commented Jan 4, 2025

No description provided.

@nquang29
Copy link
Collaborator Author

nquang29 commented Jan 4, 2025

#1570

@beastoin
Copy link
Collaborator

beastoin commented Jan 4, 2025

gm

1/ can we customize the timeout for specific routes ? e.g. GET request - 30s, POST/PATCH/DELETE 5m

2/ does the timeout work for both sync and async route func ?

3/ double checking the app to ensure they don't need the full transcript segments via /memories api. then do some necessary changes on the app, backward compatible must.

4/ snake case pls

5/ > await getConversations(limit: 10000, offset: 0, segmentLimit: 10000); // 10k for now
i see this stupid one, pls create a ticket to move it to the back-end side /export e.g.

@nquang29

@nquang29
Copy link
Collaborator Author

nquang29 commented Jan 5, 2025

1/ updated, can custom with specific routes

2/ i found docs of fastapi that fastapi automatically wraps synchronous route functions in asyncio, so its work for both sync and async route
https://fastapi.tiangolo.com/async/#very-technical-details:~:text=it%20is%20run%20in%20an%20external%20threadpool%20that%20is%20then%20awaited
https://stackoverflow.com/questions/77935269/performance-results-differ-between-run-in-threadpool-and-run-in-executor-in#:~:text=28-,Using%20run_in_threadpool(),-FastAPI%20is%20fully

3/ updated

4/ updated

5/ i'll have another MR to solve this issue

@beastoin pls help me to review again

@beastoin
Copy link
Collaborator

1/ ok
2/ ok
3/ the current apps always need the full transcripts. make sure the new api dont break the app functionality. you should check the keyword transcriptSegments in the app/ folder. for example app/lib/providers/capture_provider.dart:510 will need full transcript segments.
4/ ok
5/ ok

@nquang29

@nquang29
Copy link
Collaborator Author

3/ updated.
For each memory, I will initially fetch the first list of transcript segments based on the limit. Then, for memories that still missing transcript segments, I will call a new API with pagination to retrieve the remaining segments and complete the data for that memory.

@beastoin pls help me review

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