fix(transcribe): allow exact 4h duration and handle invalid metadata#13561
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
localstack-bot
left a comment
localstack-bot
left a comment
There was a problem hiding this comment.
Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.
|
I have read the CLA Document and I hereby sign the CLA |
purcell
left a comment
purcell
left a comment
There was a problem hiding this comment.
Thanks for taking the time to analyse, write and submit this PR. 🙏
I'll defer to the maintainers of this service for a more thorough review, but my quick feedback is that while the small fix itself may be worthwhile, the tests added here are not consistent with those elsewhere in the codebase, nor would they be maintainable in the long term. The scenarios should be possible to recreate with real sample files (without them becoming enormous), so it would be better to extend the more concrete tests instead.
| # If duration cannot be parsed, we assume it's invalid or fallback to letting it run | ||
| # But for strictness let's fail or default? | ||
| # Best practice: if we can't determine it, we can't validate it. | ||
| # Use a safe fallback or log/raise. | ||
| # Given user report of "N/A", let's fail to be safe as per AWS strictness? | ||
| # Or maybe default to fail if we assume it's a stream? | ||
| # Let's log and re-raise or handle. | ||
| # Simplest fix: treat N/A as failure? |
There was a problem hiding this comment.
This comment contains lots of questions and doesn't explain anything clearly, so it adds little to the reader's understanding of the code.
purcell
added
semver: patch
|
(Lint step apparently failing due to the issue resolved by #13710.) |
purcell
force-pushed
the
fix/transcribe-duration
branch
from
c88068b to
b50410a
Compare
purcell
force-pushed
the
fix/transcribe-duration
branch
from
b50410a to
b447580
Compare
purcell
left a comment
purcell
left a comment
There was a problem hiding this comment.
This PR is (now) just a small follow-up adjustment to #12450 by @brunodmartins last March, which already resolved #12423, and was merged by @k-a-il. This should be uncontroversial to merge now.
@kelvinvelasquez-SDE: the initial version of this PR was clearly written with the assistance of an LLM, and was quite messy, but you'll note that the end result here was a trivial two-line change, which would have been trivial to review and merge immediately. May I suggest in future emphasising minimal changes, asking maintainers' opinions, and iterating with them in conversation to come up with a good solution? This would be more satisfying for everyone. From an OSS maintainer's perspective, we want to connect with other humans and build things together, so communication is key.
4 participants
Description
Fixes #12423
This PR addresses an issue where audio files of exactly 4 hours (14400 seconds) were rejected by the
TranscribeService, despite AWS allowing files up to 4 hours. It also adds robustness to handle cases whereffprobereturns invalid duration metadata (e.g.,N/A), preventing the service from crashing.Changes
provider.py: Changed duration check from>=to>to include 14400.0s as valid.try/exceptblock to gracefully handleValueErrororTypeError, defaulting to assuming validity (or 0.0s) and logging a warning instead of raising a 500 Internal Server Error.tests/unit/services/transcribe/test_provider.pywith regression tests covering:Verification
Ran the new unit tests locally and they pass. Verified that the service no longer throws unhandled exceptions for bad metadata.