Feature sc 64229 llm sdk support audio id for openai #5
Conversation
…m-sdk-support-audio-id-for-openai
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
javascript/eslint.config.jsOops! Something went wrong! :( ESLint: 9.16.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by WalkthroughThe changes in this pull request include updates to various files in a JavaScript project, notably modifying paths and configurations related to JSON schemas and TypeScript definitions. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Assistant
participant AudioModel
participant SchemaGenerator
User->>AudioModel: Request audio response
AudioModel->>User: Generate audio response
User->>AudioModel: Follow-up query
AudioModel->>User: Generate follow-up audio response
User->>SchemaGenerator: Request TypeScript definitions
SchemaGenerator->>User: Provide TypeScript definitions
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (11)
javascript/src/schema/schema.ts (1)
153-175: Consider adding an optionalidfield toToolResultPartfor consistencyAll other message parts (
TextPart,ImagePart,AudioPart,ToolCallPart) include an optionalidfield. To maintain consistency and facilitate part identification, consider adding an optionalidfield toToolResultPart.javascript/examples/generate-audio-multiturn.ts (3)
77-77: Remove empty callback functionThe empty callback function is unnecessary. If no callback is needed, you can omit the third argument to the
playfunction.- () => {},
7-12: Consider making audio settings configurableThe audio format and voice settings are hardcoded. Consider making these configurable through environment variables or function parameters for better flexibility.
+const AUDIO_VOICE = process.env.AUDIO_VOICE || 'alloy'; +const AUDIO_FORMAT = process.env.AUDIO_FORMAT || 'mp3'; + const response = await openaiAudioModel.generate({ extra: { audio: { - voice: "alloy", - format: "mp3", + voice: AUDIO_VOICE, + format: AUDIO_FORMAT, }, },
1-4: Document external dependenciesThe example uses several external audio libraries. Consider adding:
- Documentation about these dependencies
- Installation instructions
- Browser compatibility notes
Add a comment block at the top of the file:
/** * Audio Generation Example * * Dependencies: * - audio-context: Provides WebAudio API context * - audio-decode: Decodes audio data * - audio-play: Handles audio playback * * Note: This example requires a browser environment or appropriate polyfills * for WebAudio API support. */javascript/src/utils/audio.utils.ts (1)
13-20: Consider optimizing Int16Array handlingWhile the Int16Array handling is correct, we could optimize the type checking and conversion.
Consider this more concise implementation:
export function arrayBufferToBase64( arrayBuffer: ArrayBuffer | Float32Array | Int16Array, ): string { - if (arrayBuffer instanceof Float32Array) { - arrayBuffer = floatTo16BitPCM(arrayBuffer); - } else if (arrayBuffer instanceof Int16Array) { - arrayBuffer = arrayBuffer.buffer as ArrayBuffer; - } + const buffer = arrayBuffer instanceof Float32Array + ? floatTo16BitPCM(arrayBuffer) + : arrayBuffer instanceof Int16Array + ? arrayBuffer.buffer as ArrayBuffer + : arrayBuffer;javascript/test/openai.test.ts (2)
156-209: Consider improving test organization and coverageWhile the test coverage is good, we could improve the organization and add more edge cases.
Consider:
- Adding negative test cases (invalid audio format, missing ID)
- Using test.describe for better grouping of audio-related tests
- Adding assertions for the actual ID format/structure
Example structure:
test.describe('Audio generation', () => { test('should generate audio with valid ID', async (t) => { // existing test }); test('should handle invalid audio format', async (t) => { // new test }); test('should maintain audio ID in conversation', async (t) => { // existing assistant audio part test }); });
Model ID 'gpt-4o' appears to be incorrect
The model ID "gpt-4o" used in tests and examples appears to be incorrect. The README shows that standard OpenAI model IDs should be used (e.g., "gpt-3.5-turbo"). The "gpt-4o" and "gpt-4o-audio-preview" are non-standard model IDs that don't match OpenAI's naming convention.
javascript/test/openai.test.ts: Uses incorrect model ID "gpt-4o"javascript/examples/model.ts: Uses incorrect model IDs "gpt-4o" and "gpt-4o-audio-preview"🔗 Analysis chain
Line range hint
13-13: Verify model ID configurationThe test uses 'gpt-4o' which seems like a placeholder. Verify if this is the correct model ID.
Let me search for more context about these model IDs.
Let me check if there's any documentation or configuration files that might indicate the correct model IDs.
Let me check the content of the README file for any model ID documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for actual model IDs used in the codebase rg -g '!test/*' "modelId.*gpt-4"Length of output: 328
Script:
#!/bin/bash # Search for model ID patterns in the entire codebase rg "gpt-4" -A 2 -B 2Length of output: 1439
Script:
#!/bin/bash # Search for documentation or configuration files fd -e md -e json -e yaml -e yml | xargs rg -l "gpt-"Length of output: 73
Script:
#!/bin/bash cat javascript/README.mdLength of output: 689
javascript/schema.json (1)
541-544: Consider adding more modalitiesThe Modality enum currently only supports 'text' and 'audio'. Consider adding 'image' for future expansion.
"type": "string", - "enum": ["text", "audio"] + "enum": ["text", "audio", "image"]schema/schema.json (1)
84-91: Add validation constraints for audio parameters.Consider adding minimum and maximum value constraints for
sampleRateandchannelsto prevent invalid values.Apply this diff to add the constraints:
"sampleRate": { "type": "integer", + "minimum": 8000, + "maximum": 192000, "description": "The sample rate of the audio. E.g. 44100, 48000." }, "channels": { "type": "integer", + "minimum": 1, + "maximum": 8, "description": "The number of channels of the audio. E.g. 1, 2." },javascript/src/schema/schema.json (1)
1-666: Consider using a symbolic link instead of duplicating the schema.The schema file is identical to
schema/schema.json. Consider using a symbolic link to avoid maintaining duplicate files.README.md (1)
7-7: Add missing article for better readability.Apply this diff to improve the grammar:
-The specification is expressed using JSON schema and can be found in [schema.json](../schema/schema.json). +The specification is expressed using a JSON schema and can be found in [schema.json](../schema/schema.json).🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: You might be missing the article “a” here.
Context: ...s. The specification is expressed using JSON schema and can be found in [schema.json...(AI_EN_LECTOR_MISSING_DETERMINER_A)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
README.md(1 hunks)javascript/eslint.config.js(1 hunks)javascript/examples/generate-audio-multiturn.ts(1 hunks)javascript/generate.js(1 hunks)javascript/openapi-ts.config.ts(0 hunks)javascript/package.json(3 hunks)javascript/schema.json(1 hunks)javascript/src/anthropic/anthropic.ts(9 hunks)javascript/src/cohere/cohere.ts(6 hunks)javascript/src/errors/errors.ts(1 hunks)javascript/src/google/google.ts(9 hunks)javascript/src/index.ts(1 hunks)javascript/src/mistral/mistral.ts(9 hunks)javascript/src/models/language-model.ts(1 hunks)javascript/src/openai/openai.ts(11 hunks)javascript/src/openai/types.ts(1 hunks)javascript/src/schema/index.ts(1 hunks)javascript/src/schema/schema.json(1 hunks)javascript/src/schema/schema.ts(3 hunks)javascript/src/schemas/index.ts(0 hunks)javascript/src/schemas/schemas.gen.ts(0 hunks)javascript/src/utils/audio.utils.ts(2 hunks)javascript/src/utils/message.utils.ts(1 hunks)javascript/src/utils/stream.utils.ts(7 hunks)javascript/src/utils/usage.utils.ts(1 hunks)javascript/test/openai.test.ts(2 hunks)package.json(1 hunks)patches/json-schema-to-typescript+15.0.3.patch(1 hunks)schema/schema.json(1 hunks)schemas/openapi.json(0 hunks)
💤 Files with no reviewable changes (4)
- javascript/src/schemas/index.ts
- javascript/openapi-ts.config.ts
- schemas/openapi.json
- javascript/src/schemas/schemas.gen.ts
✅ Files skipped from review due to trivial changes (3)
- javascript/src/schema/index.ts
- javascript/src/models/language-model.ts
- javascript/src/utils/message.utils.ts
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~7-~7: You might be missing the article “a” here.
Context: ...s. The specification is expressed using JSON schema and can be found in [schema.json...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
🔇 Additional comments (44)
javascript/src/schema/schema.ts (4)
52-59: TextPart interface is correctly defined
The TextPart interface accurately represents text content with required type and text fields, and an optional id.
Line range hint 95-119: AudioPart interface properly defines audio content
The AudioPart interface includes necessary properties such as type, audioData, and optional fields like container, encoding, sampleRate, channels, transcript, and id, ensuring comprehensive representation of audio content.
182-185: UserMessage interface is correctly defined
The UserMessage interface appropriately includes role and content fields, with proper types allowing TextPart, ImagePart, and AudioPart.
192-195: AssistantMessage interface is well-structured
The AssistantMessage interface correctly defines the role and content fields, supporting TextPart, ToolCallPart, and AudioPart types.
javascript/src/mistral/mistral.ts (4)
3-7: Importing specific error classes improves error handling
The addition of InvalidValueError, ModelUnsupportedMessagePart, and NotImplementedError enhances the specificity and clarity of error handling throughout the module.
Line range hint 167-206: Enhanced error handling in message conversion improves robustness
The updated error handling in convertToMistralMessages, including the use of specific error classes and exhaustive type checking, ensures that unsupported message parts and unexpected values are properly caught and reported.
249-255: Proper handling of unsupported audio parts in user messages
Throwing ModelUnsupportedMessagePart when encountering audio parts in user messages clearly indicates that audio content is not supported by the Mistral model, maintaining consistency in error handling.
333-335: Exhaustive type checking for responseFormat enhances reliability
Using exhaustive checks and throwing InvalidValueError for unexpected responseFormat types ensures that only supported formats are processed.
javascript/src/index.ts (1)
2-2: Updated exports to reflect schema restructuring
The export of schema.js correctly reflects the restructuring of schema definitions, ensuring that the correct interfaces and types are exposed.
javascript/generate.js (1)
7-19: Script correctly implements schema copying and TypeScript generation
The script successfully copies the JSON schema and compiles it into TypeScript definitions, automating the generation process for maintaining up-to-date type definitions.
javascript/eslint.config.js (1)
17-17: Updated allowDefaultProject to exclude deprecated TypeScript config
Removing "openapi-ts.config.ts" from allowDefaultProject aligns with the removal of the corresponding configuration file, ensuring ESLint does not attempt to use a non-existent project file.
javascript/src/errors/errors.ts (2)
1-5: LGTM! Well-structured error class for handling unsupported message parts.
The error message format is clear and the implementation is correct.
7-18: LGTM! Well-implemented error class with proper typing.
The error message construction is clear and the implementation includes helpful extra message support.
package.json (1)
9-9: LGTM! Dependency updates and new tools look good.
The addition of patch-package and json-schema-to-typescript along with the version updates are appropriate for the feature implementation. The postinstall script ensures patches are applied consistently.
Also applies to: 12-36
javascript/src/openai/types.ts (2)
Line range hint 1-22: LGTM! Well-documented audio handling options.
The comments clearly explain the audio ID limitations and the rationale behind the audio-to-text conversion option. This is particularly important for the 4-hour expiration constraint.
34-36: LGTM! Improved flexibility in token counting.
The change from hardcoded values to number types allows for dynamic token counts, which is more appropriate for varying content lengths.
javascript/src/utils/usage.utils.ts (1)
2-2: Verify schema import path consistency across the codebase
The import path change from ../schemas/types.gen.js to ../schema/index.js appears to be part of a larger schema restructuring effort.
✅ Verification successful
Import path migration to schema/index.js is consistent across the codebase
The verification results show that:
- No references to the old path
schemas/types.gen.jswere found in the codebase - The new import path
schema/index.jsis consistently used across multiple files in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old schema path
# and verify consistency of new schema imports
echo "Checking for old schema references..."
rg -l "schemas/types\.gen\.js"
echo "Verifying new schema import consistency..."
rg -l "schema/index\.js"
Length of output: 594
javascript/src/utils/audio.utils.ts (1)
1-1: LGTM: Import path updated as part of schema restructuring
The import path change aligns with the broader schema restructuring effort mentioned in the PR.
javascript/package.json (2)
87-96: LGTM: Proper schema.json exports configuration
The exports configuration correctly exposes schema.json for both ESM and CommonJS environments, maintaining compatibility.
Also applies to: 121-122
4-4: Verify schema generation and build process
The changes to build and generate scripts look correct, but we should verify the schema generation process.
Also applies to: 7-8
✅ Verification successful
Let me gather more information about the schema generation process.
Let me check if the schema file exists in the expected source location and verify the schema directory structure.
Schema generation and build process are working correctly
The verification shows that:
- Schema exists in the repository root at
schema/schema.json - The generate script correctly copies it to
javascript/src/schema/schema.json - TypeScript types are generated as
schema.ts - Build script is configured to copy schema.json to both ESM and CommonJS dist folders
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that generate.js exists and schema.json is properly copied during build
fd -e js "generate.js$"
test -f src/schema/schema.json && echo "Schema exists" || echo "Schema missing"
Length of output: 120
Script:
#!/bin/bash
# Check the content of generate.js and package.json build scripts
cat javascript/generate.js
echo "---"
cat javascript/package.json | jq '.scripts'
Length of output: 932
Script:
#!/bin/bash
# Check if schema exists in the repository root and javascript directory structure
test -f schema/schema.json && echo "Schema exists in root" || echo "Schema missing in root"
ls -la javascript/schema/ 2>/dev/null || echo "No schema dir in javascript root"
ls -la javascript/src/schema/ 2>/dev/null || echo "No schema dir in javascript/src"
Length of output: 516
javascript/test/openai.test.ts (1)
111-111: LGTM: Audio ID verification in tests
The assertions properly verify that audio parts include an ID in both generate and stream scenarios.
Also applies to: 153-153
javascript/src/utils/stream.utils.ts (5)
34-36: LGTM: Proper propagation of part IDs
The addition of ID handling ensures that part identifiers are correctly maintained during stream accumulation.
111-111: LGTM: Consistent ID handling in text parts
The conditional inclusion of ID using spread operator maintains consistency with the stream accumulation changes.
123-123: LGTM: Consistent ID handling in tool-call parts
The conditional inclusion of ID maintains consistency across different part types.
139-139: LGTM: Consistent ID handling in audio parts
The conditional inclusion of ID maintains consistency across all part types.
148-153: LGTM: Improved type safety in default case
The exhaustive type checking ensures that all part types are properly handled at compile time.
javascript/src/anthropic/anthropic.ts (3)
178-178: LGTM: Improved error specificity for unsupported audio parts
Using ModelUnsupportedMessagePart provides clearer error context compared to generic errors.
181-183: LGTM: Consistent error handling in type validation
The use of InvalidValueError with property names and values provides better debugging context.
Also applies to: 240-242, 253-255
345-347: LGTM: Clear error handling for unimplemented features
The use of NotImplementedError clearly indicates unimplemented functionality with specific context.
Also applies to: 383-385, 413-415
javascript/src/cohere/cohere.ts (3)
191-193: LGTM: Improved error handling for invalid message roles
Using InvalidValueError provides better context for debugging invalid message roles.
221-223: LGTM: Consistent handling of unsupported message types
Efficiently handles multiple unsupported types with clear error messaging.
253-255: LGTM: Consistent error handling throughout
The use of InvalidValueError maintains consistency and improves error context across all validation points.
Also applies to: 355-357
javascript/src/google/google.ts (4)
17-17: LGTM: Import changes improve error handling and maintain consistency
The changes improve error handling by importing specific error classes and update the schema import path for consistency.
Also applies to: 33-33
210-210: LGTM: Improved error handling with exhaustive type checking
The changes enhance error handling by using InvalidValueError with exhaustive type checking for better error messages and type safety.
Also applies to: 233-233
Line range hint 417-439: LGTM: Enhanced error handling for unsupported features
The changes properly handle unsupported features by using NotImplementedError with specific error contexts.
Also applies to: 450-450, 454-454
497-500: LGTM: Consistent error handling in delta mapping
The error handling in delta mapping is consistent with the rest of the file.
javascript/src/openai/openai.ts (4)
238-244: Critical: Validate audio ID presence
The code correctly validates that audio parts must have an ID, which is essential for OpenAI's audio support.
205-213: LGTM: Improved type safety for message parameters
The changes enhance type safety by using Omit to properly type the content property.
Line range hint 523-531: LGTM: Proper audio part mapping
The code correctly maps audio parts with their IDs and formats.
Line range hint 578-586: LGTM: Proper audio delta handling
The code correctly handles audio deltas with optional ID and data fields.
javascript/schema.json (4)
1-4: LGTM: Well-structured schema definition
The schema is properly defined with title and schema version.
48-57: LGTM: Comprehensive audio format definitions
The schema properly defines audio encodings and container formats with clear descriptions.
58-91: LGTM: Well-defined audio part schema
The AudioPart schema:
- Includes all necessary properties
- Makes only type and audioData required
- Prevents additional properties
270-302: LGTM: Proper audio delta schema
The AudioPartDelta schema correctly defines the structure for streaming audio responses.
| import { compileFromFile } from "json-schema-to-typescript"; | ||
| import { join } from "path"; | ||
|
|
||
| const __dirname = import.meta.dirname; |
There was a problem hiding this comment.
Incorrect usage of import.meta.dirname; should use import.meta.url to derive __dirname
The import.meta object does not have a dirname property. To get the directory name in an ES module, you should use import.meta.url with fileURLToPath and dirname from the path module.
Apply this diff to correct the issue:
+import { dirname, join } from "path";
+import { fileURLToPath } from "url";
-const __dirname = import.meta.dirname;
+const __filename = fileURLToPath(import.meta.url);
+const __dirname = dirname(__filename);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const __dirname = import.meta.dirname; | |
| import { dirname, join } from "path"; | |
| import { fileURLToPath } from "url"; | |
| const __filename = fileURLToPath(import.meta.url); | |
| const __dirname = dirname(__filename); |
| - ast.comment = ast.comment ? `${ast.comment}\n\n${comment}` : comment; | ||
| + ast.comment = ast.comment ? `${ast.comment}\n\n${comment}` : ''; |
There was a problem hiding this comment.
Reconsider discarding pattern property reference comments
The change to use an empty string fallback instead of the pattern property reference comment might lead to loss of useful type documentation. These comments help developers understand the relationship between interfaces and their parent schemas.
Consider keeping the original behavior:
- ast.comment = ast.comment ? `${ast.comment}\n\n${comment}` : '';
+ ast.comment = ast.comment ? `${ast.comment}\n\n${comment}` : comment;Also applies to: 22-23
| // @ts-expect-error: it works ok? | ||
| context: audioContext, |
There was a problem hiding this comment.
Remove @ts-expect-error and fix type definitions
The type assertion bypass indicates a mismatch between the audio-play library's type definitions and the audio-context usage. Instead of suppressing the type error, consider:
- Adding proper type definitions for the audio-play library
- Using a type-safe audio playback library
| if (audioPart2) { | ||
| const audioBuffer2 = await decodeAudio( | ||
| Buffer.from(audioPart2.audioData, "base64"), | ||
| ); | ||
| const playback2 = play( | ||
| audioBuffer2, | ||
| { | ||
| // @ts-expect-error: it works ok? | ||
| context: audioContext, | ||
| }, | ||
| () => {}, | ||
| ); | ||
| playback2.play(); | ||
| } |
There was a problem hiding this comment.
Add error handling and resource cleanup
The audio processing code lacks error handling and resource cleanup. Consider:
- Adding try-catch blocks for audio operations
- Implementing proper cleanup of audio resources
- Adding error handling for invalid audio data
Here's a suggested improvement:
if (audioPart2) {
+ try {
const audioBuffer2 = await decodeAudio(
Buffer.from(audioPart2.audioData, "base64"),
);
const playback2 = play(
audioBuffer2,
{
context: audioContext,
},
- () => {},
);
playback2.play();
+ // Cleanup when done
+ playback2.on('end', () => {
+ audioContext.close();
+ });
+ } catch (error) {
+ console.error('Error processing audio:', error);
+ if (audioContext.state === 'running') {
+ audioContext.close();
+ }
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| ], | ||
| }); | ||
|
|
||
| console.dir(response, { depth: null }); |
There was a problem hiding this comment.
This is just example code. We can keep it.
| ], | ||
| }); | ||
|
|
||
| console.dir(response2, { depth: null }); |
What does this PR do?
Type of change
This pull request is a
Which introduces
How should this be manually tested?
Check test case. Or test with
javascript/examples/generate-audio-multiturn.tsWhat are the requirements to deploy to production?
Any background context you want to provide beyond Shortcut?
Lots of changes due to some refactoring (mainly tooling change of openapi -> json schema). only relevant commit for audio id support: 97e7780
Screenshots (if appropriate)
Loom Video (if appropriate)
trim.883007DD-2F52-4932-8898-CF5B880FF622.MOV
Any Security implications
xxx
Summary by CodeRabbit
Release Notes
New Features
schema.json) for the language model SDK, defining various message parts and their properties.Bug Fixes
Documentation
README.mdto reflect the new schema file location.Chores
package.jsonfor improved functionality and performance.