Skip to content

chore: add timeout for api#1638

Merged
beastoin merged 3 commits intoBasedHardware:mainfrom
nquang29:timeout_1570
Feb 17, 2025
Merged

chore: add timeout for api#1638
beastoin merged 3 commits intoBasedHardware:mainfrom
nquang29:timeout_1570

Conversation

@nquang29
Copy link
Copy Markdown
Collaborator

@nquang29 nquang29 commented Jan 4, 2025

No description provided.

@nquang29
Copy link
Copy Markdown
Collaborator Author

nquang29 commented Jan 4, 2025

#1570

@beastoin
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@beastoin
Copy link
Copy Markdown
Collaborator

beastoin commented Feb 6, 2025

3/ to reduce the risk of break changes lets discard the limit_segment logic.
6/ what is the timeout of the wss /v3/listen API ? sry i worry that the new timeout logic will affect the websocket connections.

@nquang29 ~

@nquang29
Copy link
Copy Markdown
Collaborator Author

3/ discarded all logic about segment limit
6/ the new logic does not affect to ws, it's only work for http request.
I found that the ws functionality already handled the timeout logic. It will work if the NO_SOCKET_TIMEOUT env variable is None, and i have also moved the timeout_second value to an env variable.
@beastoin

@beastoin
Copy link
Copy Markdown
Collaborator

3/ ✅ ok
6/ ✅ ok
7/ discard the WS_TIMEOUT related pls, this timeout is at the functional level. we only focus on the method level. https://github.com/BasedHardware/omi/pull/1638/files#diff-7c62653a4a8a3f0f493b5372a547288fac05a7381701740be55da9a93197d0c9R557

@nquang29 last commit pls sir.

@nquang29
Copy link
Copy Markdown
Collaborator Author

7/ i have discarded it
@beastoin

@beastoin beastoin merged commit 65bcc0b into BasedHardware:main Feb 17, 2025
@beastoin
Copy link
Copy Markdown
Collaborator

7/ ✅ ok

lgtm @nquang29 / congratulation 🚀

@beastoin
Copy link
Copy Markdown
Collaborator

Screenshot 2025-02-17 at 16 46 57

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