-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat: add support for nova 3 and keyterms #359
Conversation
Warning Rate limit exceeded@naomi-lgbt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request updates the transcription and live agent configuration to use the "nova-3" models instead of "nova-2". In addition, a new keyterms option is introduced across various files, and the type definitions are expanded to support the updated configurations. Error handling logic is added in client configuration methods to enforce that keyterms are only used when the model starts with "nova-3". Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as Client Configurator
participant V as Validator
participant E as Error Handler
U->>C: Call configure(options)
C->>V: Validate options (model & keyterms)
alt keyterms provided and model not valid
V-->>C: Return error condition
C->>E: Trigger DeepgramError("Keyterms are only supported with the Nova 3 models.")
C-->>U: Throw error
else Valid configuration
V-->>C: Validation successful
C-->>U: Proceed with configuration
end
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
src/packages/ListenRestClient.ts (3)
82-115
: Add keyterms validation to transcribeFile method.For consistency, add the same keyterms validation to the
transcribeFile
method as it also accepts transcription options.if (options !== undefined && "callback" in options) { throw new DeepgramError( "Callback cannot be provided as an option to a synchronous transcription. Use `transcribeUrlCallback` or `transcribeFileCallback` instead." ); } + if (options?.keyterms?.length && !options.model?.startsWith("nova-3")) { + throw new DeepgramError("Keyterms are only supported with the Nova 3 models."); + } + const requestUrl = this.getRequestUrl(endpoint, {}, { ...{}, ...options });
126-158
: Add keyterms validation to transcribeUrlCallback method.For consistency, add the same keyterms validation to the
transcribeUrlCallback
method as it also accepts transcription options.} else { throw new DeepgramError("Unknown transcription source type"); } + if (options?.keyterms?.length && !options.model?.startsWith("nova-3")) { + throw new DeepgramError("Keyterms are only supported with the Nova 3 models."); + } + const requestUrl = this.getRequestUrl(
169-201
: Add keyterms validation to transcribeFileCallback method.For consistency, add the same keyterms validation to the
transcribeFileCallback
method as it also accepts transcription options.} else { throw new DeepgramError("Unknown transcription source type"); } + if (options?.keyterms?.length && !options.model?.startsWith("nova-3")) { + throw new DeepgramError("Keyterms are only supported with the Nova 3 models."); + } + const requestUrl = this.getRequestUrl(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/node-agent-live/index.js
(1 hunks)examples/node-live/index.js
(2 hunks)examples/node-prerecorded/index.js
(2 hunks)src/lib/types/AgentLiveSchema.ts
(2 hunks)src/lib/types/TranscriptionSchema.ts
(1 hunks)src/packages/AgentLiveClient.ts
(2 hunks)src/packages/ListenLiveClient.ts
(2 hunks)src/packages/ListenRestClient.ts
(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
src/packages/AgentLiveClient.ts
[error] 6-6: Module not found: Can't resolve '../lib/errors.js' in '/home/runner/work/deepgram-js-sdk/deepgram-js-sdk/src/packages'
🔇 Additional comments (10)
examples/node-prerecorded/index.js (2)
14-15
: LGTM! Good choice of keyterm.The keyterm "spacewalk" is well-chosen as it matches the content of the audio file being transcribed (dpgr.am/spacewalk.wav).
32-33
: LGTM! Consistent configuration.The configuration matches the transcribeUrl function, maintaining consistency across both transcription methods.
examples/node-agent-live/index.js (1)
30-31
: LGTM! Well-structured agent configuration.The model update and keyterm addition are properly configured within the agent's listen property. The keyterm "spacewalk" matches the audio content from dpgr.am/spacewalk.wav.
examples/node-live/index.js (1)
13-24
: LGTM! Well-documented configuration with appropriate keyterm.The changes are well-implemented:
- Good documentation explaining the keyterms feature and its benefits
- The keyterm "BBC" is appropriate for the BBC World Service stream
src/lib/types/AgentLiveSchema.ts (2)
29-31
: LGTM! Added new model types.The new nova-3 models are properly added to the ListenModel type.
157-173
: LGTM! Well-designed type safety for nova-3 configuration.Excellent use of TypeScript's discriminated unions to enforce that keyterms are only used with nova-3 models. The JSDoc comments provide good documentation for the configuration options.
src/lib/types/TranscriptionSchema.ts (1)
97-100
: LGTM!The new
keyterms
property is well-documented and follows the same pattern as other similar properties in the interface.src/packages/ListenLiveClient.ts (1)
38-43
: LGTM!The validation ensures that keyterms are only used with nova-3 models, with a clear error message. The check is performed early in the constructor which is good for fail-fast behavior.
src/packages/AgentLiveClient.ts (1)
123-126
: LGTM!The validation ensures that keyterms are only used with nova-3 models, with a clear error message. The
@ts-expect-error
comment is well-documented, explaining why the runtime check is necessary for JavaScript consumers.src/packages/ListenRestClient.ts (1)
55-57
: LGTM!The validation ensures that keyterms are only used with nova-3 models, with a clear error message.
@@ -34,6 +35,13 @@ export class ListenLiveClient extends AbstractLiveClient { | |||
) { | |||
super(options); | |||
|
|||
if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was debating if we needed this exception in the SDKs as our API enforcing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed in Slack - moot point, we're fine either way as these SDKs will soon be approached differently.
Summary by CodeRabbit