Skip to content

Conversation

@Sang-it
Copy link
Member

@Sang-it Sang-it commented Jan 22, 2026

No description provided.

@Sang-it Sang-it marked this pull request as ready for review January 22, 2026 05:03
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 22, 2026

Greptile Summary

Consolidated embedding functionality from the separate umem_embeddings crate into the existing umem_ai crate, removing the Cloudflare-specific implementation in favor of a unified embedding API supporting OpenAI and Amazon Bedrock providers.

  • Removed the entire umem_embeddings crate and its Cloudflare implementation
  • Added EmbeddingModel struct and embed() function to umem_ai with OpenAI and Bedrock support in configuration
  • Migrated MemoryController to use the new umem_ai embedding API
  • Updated config to replace Cloudflare-specific enum with generic EmbeddingModel struct

Critical Issue: The EmbeddingModel::get_model() allows creating an OpenAI provider, but OpenAIProvider doesn't implement the Embeds trait, causing a runtime panic when embeddings are requested with OpenAI configuration.

Confidence Score: 1/5

  • This PR contains a critical logic error that will cause runtime panics when using OpenAI embeddings
  • The refactoring is well-structured and the migration from umem_embeddings to umem_ai is clean, but there's a critical bug where EmbeddingModel::get_model() can create an OpenAI provider that lacks the required Embeds trait implementation. This will cause the application to panic at runtime with unimplemented!() when users configure OpenAI for embeddings. The code currently only works with Amazon Bedrock.
  • Pay close attention to crates/umem_ai/src/lib.rs - the OpenAI embedding support must be either implemented or removed from the configuration options

Important Files Changed

Filename Overview
crates/umem_ai/src/lib.rs added EmbeddingModel initialization with OpenAI and Bedrock support, but do_embed only handles Bedrock causing runtime panic for OpenAI
crates/umem_config/src/lib.rs replaced Cloudflare-specific EmbeddingModel enum with generic struct supporting multiple providers
crates/umem_controller/src/create_memory.rs migrated from umem_embeddings to umem_ai embed API with workaround for slice conversion
crates/umem_controller/src/search_memory.rs migrated from umem_embeddings to umem_ai embed API for all search operations
crates/umem_memory_machine/src/lib.rs migrated from umem_embeddings trait to umem_ai::EmbeddingModel

Sequence Diagram

sequenceDiagram
    participant Client
    participant MemoryController
    participant EmbeddingModel
    participant AIProvider
    participant BedrockProvider
    participant VectorStore

    Client->>MemoryController: create_memory(request)
    MemoryController->>MemoryController: build memory with LLM annotations
    MemoryController->>EmbeddingModel: build EmbeddingRequest
    MemoryController->>EmbeddingModel: embed(request)
    EmbeddingModel->>AIProvider: do_embed(request)
    
    alt AmazonBedrock
        AIProvider->>BedrockProvider: embed(request)
        BedrockProvider->>BedrockProvider: spawn parallel invoke_model tasks
        BedrockProvider-->>AIProvider: EmbeddingResponse
    else OpenAI (unimplemented)
        AIProvider-->>EmbeddingModel: panic: unimplemented!()
    end
    
    AIProvider-->>EmbeddingModel: EmbeddingResponse
    EmbeddingModel-->>MemoryController: embeddings: Vec<Vec<f32>>
    MemoryController->>MemoryController: convert to &[&[f32]] slices
    MemoryController->>VectorStore: insert(slices, memories)
    VectorStore-->>MemoryController: Ok(())
    MemoryController-->>Client: Memory
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional Comments (1)

  1. crates/umem_ai/src/lib.rs, line 283-292 (link)

    logic: do_embed only handles AmazonBedrock but EmbeddingModel::get_model() (lines 176-216) can return an OpenAI provider. This causes a runtime panic with unimplemented!() when using OpenAI for embeddings.

16 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

@vidurkhanal vidurkhanal merged commit f98bbca into main Jan 22, 2026
2 checks passed
@vidurkhanal vidurkhanal deleted the feat/switch-embed branch January 22, 2026 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants