[MM-53432] Improvements #565
Conversation
streamer45
added
2: Dev Review
| @@ -302,6 +303,9 @@ func (s *jobService) RunJob(jobType job.Type, callID, postID, jobID, authToken s | |||
| transcriberConfig.PostID = postID | |||
| transcriberConfig.TranscriptionID = jobID | |||
| transcriberConfig.AuthToken = authToken | |||
| if val := os.Getenv("MM_CALLS_TRANSCRIBER_NUM_THREADS"); val != "" { | |||
| transcriberConfig.NumThreads, _ = strconv.Atoi(val) | |||
There was a problem hiding this comment.
Should we be handling the error with a default, or assume it's correct (because its an admin-controlled env variable)? (Just curious about that kind of thing system-wide, @jupenur )
There was a problem hiding this comment.
calls-transcriber implements validation and errors on a zero value. In general we should strive to validate all inputs regardless of the level of trust assumed just to avoid mistakes resulting from bad assumptions, and to make a habit of it. That said, there'd be no direct security impact here even if there was no validation.
0/5 on whether we'd want to add some extra error handling here, too, just to make the admin UX better.
There was a problem hiding this comment.
Yes, this is really there to help me test and/or for very advanced use cases at the moment. Until we surface this through proper config I am okay with keeping it this way as the ultimate validation is done on the other side and when zero (error case) a default is used.
| name: "special chars", | ||
| input: "somefile*with??special/\\chars.mp4", | ||
| expected: "somefile_with__special__chars.mp4", | ||
| }, |
There was a problem hiding this comment.
Should we test the too-long case also?
There was a problem hiding this comment.
Yep, added on the specific function, thanks 👍
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## MM-53432 #565 +/- ##
===========================================
+ Coverage 5.68% 5.75% +0.07%
===========================================
Files 26 26
Lines 5011 5071 +60
===========================================
+ Hits 285 292 +7
- Misses 4703 4756 +53
Partials 23 23 ☔ View full report in Codecov by Sentry. |
streamer45
added
3: Reviews Complete
* handleBotGetProfileForSession * handleBotPostTranscriptions * Refactor job api, part I * Updates * Job status updates * Couple transcribing job with recording * Update offloader * Updates * Remove event * Bump Go version * Add some docs * Add e2e test for call transcriptions * Use leaner API request context * Verify transcription content * Bump calls-common * Fix error message * Rename param * Init transcriber runner only if enabled * Allow status failure report even after job has ended * Add public.JobInfo * Bump public module * Update recorder and transcriber deps * Update recording ended banner text * Add processing message * Update job info types * Fix transcription file access issues * Cache current date to avoid refetching * Update comment * [MM-53432] Improvements (#565) * Add Dismiss button to recording job post-processing message * Implement more human friendly filename for recordings and transcriptions * Support setting transcriber threads * Improvements * Include transcription post ID in metadata * Add config option to select model size (#566) * Update transcribing job max duration * Update strings * Update deps * Fix e2e imports * Use underscore to replace spaces in filenames
Summary
Some minor improvements as requested in the parent PR.