-
Notifications
You must be signed in to change notification settings - Fork 6
feat: add embeddings API support to Rust SDK #59
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
Conversation
Add create_embeddings() method supporting single and multiple text inputs with EmbeddingRequest/EmbeddingResponse types and From trait implementations for convenient input handling. Also fixes clippy warning in attestation.rs. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Deploying opensecret-sdk with
|
| Latest commit: |
698cb9b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://235f3e90.opensecret-sdk.pages.dev |
| Branch Preview URL: | https://feat-embeddings-api.opensecret-sdk.pages.dev |
📝 WalkthroughWalkthroughThis change adds embeddings API support to the OpenSecretClient by introducing new request and response types, implementing conversion traits for flexible input handling, and adding a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
Greptile SummaryAdded embeddings API support to the Rust SDK, enabling text-to-vector conversion through the Major Changes:
Implementation Quality:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as OpenSecretClient
participant API as OpenSecret API
participant Crypto as Encryption Layer
Note over Client,API: Embeddings API Flow
Client->>Client: Create EmbeddingRequest
Note right of Client: input: String or Vec<String><br/>model: "nomic-embed-text"
Client->>Crypto: Encrypt request with session key
Crypto-->>Client: Encrypted payload
Client->>API: POST /v1/embeddings<br/>(encrypted + session-id header)
Note right of Client: Uses encrypted_openai_call()<br/>Supports API key or JWT auth
API->>API: Decrypt request
API->>API: Generate embeddings
API->>API: Encrypt response
API-->>Client: Encrypted EmbeddingResponse
Client->>Crypto: Decrypt response
Crypto-->>Client: Decrypted data
Client->>Client: Parse EmbeddingResponse
Note left of Client: Returns:<br/>- data: Vec<EmbeddingData><br/>- usage: token counts<br/>- model info
Client-->>Client: Return Result<EmbeddingResponse>
|
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: 0
🧹 Nitpick comments (1)
rust/src/types.rs (1)
485-508: Excellent ergonomic design with From trait implementations.The
#[serde(untagged)]attribute onEmbeddingInputallows flexible JSON serialization (either a single string or an array of strings), and theFromtrait implementations provide convenient construction. The API feels natural and type-safe.💡 Optional: Consider adding From<&[String]> for slice references
If you want to avoid an allocation when passing a borrowed slice:
+impl From<&[String]> for EmbeddingInput { + fn from(v: &[String]) -> Self { + EmbeddingInput::Multiple(v.to_vec()) + } +}This is optional and only beneficial if callers frequently have
&[String]rather thanVec<String>.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
rust/src/attestation.rsrust/src/client.rsrust/src/types.rsrust/tests/ai_integration.rs
🧰 Additional context used
🧬 Code graph analysis (1)
rust/tests/ai_integration.rs (1)
rust/tests/api_keys.rs (1)
env(24-26)
🪛 GitHub Actions: Rust CI
rust/tests/ai_integration.rs
[error] 255-255: Guest users should not be able to access models. Test failed: test_guest_user_cannot_use_ai in ai_integration; Process exited with code 101 while running 'cargo test --all-features'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Greptile Review
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
rust/src/attestation.rs (1)
29-37: LGTM! Clippy suppression is justified.The
#[allow(clippy::derivable_impls)]attribute is appropriate here because theallow_debugfield usescfg!(feature = "mock-attestation"), which cannot be expressed in a derivedDefaultimplementation. The delegation fromnew()todefault()is good practice.rust/src/types.rs (2)
467-479: LGTM! Well-structured request type.The
EmbeddingRequestfollows OpenAI API conventions with appropriate defaults. The use ofdefault_embedding_model()for the model field provides a sensible default while allowing customization.
510-529: Response types correctly match OpenAI API structure.The
EmbeddingResponse,EmbeddingData, andEmbeddingUsagetypes accurately model the API response. UsingVec<f64>for embeddings is appropriate for the floating-point vectors returned by embedding models.rust/src/client.rs (1)
969-985: Excellent implementation following established patterns.The
create_embeddingsmethod properly reuses theencrypted_openai_callhelper, ensuring consistent encryption and authentication behavior with other OpenAI endpoints. The doc comment provides a clear usage example.rust/tests/ai_integration.rs (3)
26-42: Good improvement to test setup robustness.The try-login-first approach with fallback to registration makes the tests more resilient to repeated runs and avoids registration conflicts. The optional
nameparameter aligns with the registration API.
283-396: Comprehensive test coverage for embeddings functionality.The three test cases provide excellent coverage:
- Single input validation with dimension checks (768 for nomic-embed-text)
- Multiple inputs with proper indexing verification
- Ergonomic string conversion via
FromtraitsAll assertions verify the response structure, embedding dimensions, and usage metrics appropriately.
224-281: Address unimplemented guest access restrictions test.The
test_guest_user_cannot_use_aitest was added in this PR but expects guest users to be denied access to models and completions. However, the SDK currently has no guest-specific authorization checks—get_models()andcreate_chat_completion()use the same session-based authentication for all user types, with no logic to distinguish or restrict guests. Either implement guest access enforcement on the server/SDK side, or remove/mark this test as#[ignore]if guest restrictions are not yet supported.
Summary
Add embeddings API support to the Rust SDK.
Changes
create_embeddings()method toOpenSecretClientEmbeddingRequest,EmbeddingResponse,EmbeddingInput,EmbeddingData, andEmbeddingUsagetypesFromtrait implementations for convenientEmbeddingInputconstruction from stringsAttestationVerifier::DefaultimplTesting
All new embedding tests pass with the nomic-embed-text model (768 dimensions).
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.