fix(sync): send audio-bytes webhook during the offline sync#3930
fix(sync): send audio-bytes webhook during the offline sync#393042atomys wants to merge 13 commits intoBasedHardware:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request successfully adds the functionality to trigger webhooks for offline audio sync. The new utility functions are well-defined. However, there is a significant performance issue: the asynchronous webhook function uses a synchronous HTTP library (requests), which will block the event loop. My review includes a comment with details on how to fix this by using an asynchronous library like httpx, which was conveniently added in this PR.
d569556 to
511fce6
Compare
There was a problem hiding this comment.
Code Review
This pull request enhances the offline audio sync workflow by ensuring that developer webhooks are triggered with audio bytes even for files uploaded and processed offline. The changes correctly replace the synchronous requests library with httpx for asynchronous webhook calls. However, a critical performance issue is identified due to performing blocking file I/O within an async endpoint, which will block the server's event loop. Suggestions have been provided to refactor this using asyncio.to_thread to prevent performance degradation, aligning with best practices for asynchronous applications. Additionally, a suggestion to narrow a broad exception catch has been included to improve error handling.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request enhances the offline audio sync workflow by triggering webhooks with audio bytes. The implementation is solid, correctly using asyncio.to_thread for blocking I/O and httpx for asynchronous network requests. I've provided one suggestion in backend/routers/sync.py to centralize error handling, which will improve code clarity and maintainability. After addressing this, the PR should be ready to merge.
There was a problem hiding this comment.
Code Review
This pull request successfully adds the functionality to send audio bytes via webhook for offline-synced files. It correctly refactors a synchronous network call to be asynchronous using httpx, which is a crucial improvement. My main feedback is to enhance performance by parallelizing the file reading operations. The current implementation reads files one by one, which can be slow. By processing them concurrently, the overall sync time can be significantly reduced.
There was a problem hiding this comment.
Code Review
This pull request adds a valuable feature to send audio-bytes webhooks for offline-synced files. The implementation is well-structured, using asyncio for concurrent processing. My review includes a few suggestions to improve performance and memory usage. Specifically, I recommend changing the webhook data type from bytearray to bytes to avoid unnecessary data copying, and running the webhook triggers as a background task to prevent blocking the main API response. These changes will make the new feature more efficient and robust.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request enhances the offline audio sync workflow by ensuring that developer webhooks are triggered with audio bytes for offline-processed files. It correctly introduces a timestamp parameter to provide better context for these webhooks. A key improvement is the switch to httpx for asynchronous HTTP requests in send_audio_bytes_developer_webhook, which prevents blocking the event loop and aligns with best practices for async applications. The documentation has also been clearly updated. I have one high-severity suggestion to prevent a potential runtime error.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request successfully enhances the offline audio sync workflow by triggering developer webhooks with audio bytes for offline-processed files. The addition of a timestamp query parameter is a valuable enhancement for providing accurate recording times. The implementation correctly leverages asyncio for concurrent processing of audio files and non-blocking webhook notifications. A significant improvement is the migration from the synchronous requests library to the asynchronous httpx for sending webhooks, which is crucial for the performance of an async application. My review includes a suggestion to extend this asynchronous pattern to another webhook function within the same file to ensure consistency and further improve performance.
|
cool, the audio bytes developer webhook is designed for real-time use cases when a developer receives the audio now, not from the past. adding support for past audio is cool, but idk who would really need it and why? if you still want to go with it, i'd like to suggest adding a new option to developer settings to allow devs to receive pre-recorded audio bytes via the audio bytes webhook so their current setup won't break. |
|
Hi @beastoin, thanks for the feedback. I will explain the "broken" workflow who decide me to implement this possibility with a scenario : I'm an user of hardware with offline sync, like a Limitless Pendant, I use developer webhook to send audio to Google Cloud (example given here). When Omi app arent working, or phone is out of range of the Pendant, audio cannot be retrieve anymore, causing a loss of raw data. Having the possibility to retrieve lost data from offline sync are the quick-win solution. Adding an option to send or not offline sync and have the |
|
Understood. Please go ahead with the option and the timestamp, test it carefully, then we will proceed. By "option," you should check how the current webhook works, and we need to update both the app and the backend. Feel free to ask me any questions if there are any blockers. |
Resolved conflict in app/lib/pages/settings/developer.dart: - Kept our checkbox feature for offline sync opt-in - Used upstream's localized label (context.l10n.intervalSeconds)
|
Finally have free time to add the opt-in setting as you suggested. New checkbox in Developer Settings (mobile + desktop) to enable audio bytes webhook during offline sync. Disabled by default so nothing breaks for existing setups. When enabled, the webhook also includes the timestamp param so devs know when the audio was actually recorded. Docs updated too. Btw, I'm not super familiar with how to build a local demo to test this end-to-end (mobile mock) - any pointers on how you usually test this kind of change? |
|
Hey 👋 — checking in on this one! I saw you asked about how to test this end-to-end. For backend changes, you can set up a local dev environment following https://docs.omi.me/docs/developer/backend/Backend_Setup — from there you can capture terminal output or curl results showing the webhook firing during offline sync. A quick demo (even terminal logs) showing the feature working would help move this forward. In the AI era, writing code is the easy part — proof it works end-to-end is the minimum bar for a good PR. Let us know if you need any help with the setup! 🙏 |
This pull request enhances the offline audio sync workflow by ensuring that developer webhooks are triggered with audio bytes even for files uploaded and processed offline.
I also add a new query params to send the
timestampof the file to provide the right "when is recorded" to webhooksIt introduces utility functions to read WAV files as PCM data and schedules asynchronous webhook notifications for each synced audio file.
Resolve #3846