Embedder#1
Conversation
- Introduced `EmbeddingConfig` struct for managing embedding model configurations. - Implemented `EmbeddingClient` for handling model initialization and embedding generation. - Added environment variable support for model selection, cache directory, and pool size. - Created a main function to demonstrate model loading and embedding generation with sample texts. - Integrated `fastembed` library for embedding functionalities.
… and add integration tests
…rkflow, and README documentation
|
@copilot create a pr fixing the dockerfile |
|
@ArnabChatterjee20k I've opened a new pull request, #2, to work on those changes. Once the pull request is ready, I'll request review from you. |
Agent-Logs-Url: https://github.com/appwrite/embedding/sessions/4f5decfe-9143-42bd-b0e6-811ed2616fcb Co-authored-by: ArnabChatterjee20k <83803257+ArnabChatterjee20k@users.noreply.github.com>
Agent-Logs-Url: https://github.com/appwrite/embedding/sessions/4f5decfe-9143-42bd-b0e6-811ed2616fcb Co-authored-by: ArnabChatterjee20k <83803257+ArnabChatterjee20k@users.noreply.github.com>
Agent-Logs-Url: https://github.com/appwrite/embedding/sessions/5bc8a2a9-7dfa-400d-9834-850b8f96eb8d Co-authored-by: ArnabChatterjee20k <83803257+ArnabChatterjee20k@users.noreply.github.com>
Set default runtime model cache path and fix Docker CI compatibility
Greptile SummaryThis PR introduces a new Rust microservice that serves text embeddings over HTTP using the
Confidence Score: 4/5Safe to merge with one test correctness fix — the alias-rejection test never executes its assertion in CI and should be restructured or marked #[ignore]. The alias-rejection test in tests/embed_e2e.rs swallows the model-loading error with .ok() and wraps its assertions in if let Some(client), so on a CI runner without a cached model the entire assertion block is skipped and the test passes vacuously. This means the alias-validation code path is not exercised in CI at all, despite appearing in the test suite. tests/embed_e2e.rs — the embed_unknown_alias_returns_error test needs restructuring to actually run its assertion. src/model.rs — the dimension() function should add an explicit arm for EmbeddingGemma300M. Important Files Changed
Reviews (2): Last reviewed commit: "feat: implement multi-model support in E..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| on: | ||
| push: | ||
| branches: [main] | ||
| pull_request: |
There was a problem hiding this comment.
main, but the repository's default branch (and the base for this PR) is master. As-is, the test and docker jobs will never fire on a direct push to master, so merges to the primary branch go unvalidated by CI.
| on: | |
| push: | |
| branches: [main] | |
| pull_request: | |
| on: | |
| push: | |
| branches: [master] | |
| pull_request: |
| #[tokio::main] | ||
| async fn main() -> Result<(), Box<dyn std::error::Error + Send + Sync>> { | ||
| tracing_subscriber::fmt() | ||
| .with_env_filter( | ||
| tracing_subscriber::EnvFilter::try_from_default_env() | ||
| .unwrap_or_else(|_| tracing_subscriber::EnvFilter::new("info")), | ||
| ) | ||
| .init(); | ||
|
|
||
| let config = EmbeddingConfig::from_env(); | ||
| let client = Arc::new(EmbeddingClient::new(config)?); | ||
| let state = AppState { client }; | ||
|
|
||
| let app = Router::new().route("/embed", post(embed)).with_state(state); | ||
|
|
||
| let addr = std::env::var("BIND_ADDR").unwrap_or_else(|_| "0.0.0.0:3000".to_string()); | ||
| let listener = tokio::net::TcpListener::bind(&addr).await?; | ||
| tracing::info!("listening on {}", addr); | ||
| axum::serve(listener, app).await?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
axum::serve is awaited directly with no signal hook. When the container receives SIGTERM, the process is killed immediately and any in-flight embedding requests are dropped. Axum supports a .with_graceful_shutdown(signal) overload that drains connections before exit.
| #[tokio::test(flavor = "multi_thread")] | ||
| async fn embed_unknown_alias_returns_error() { | ||
| // No model loading on this path — fails the alias check first. | ||
| let client = EmbeddingClient::new(small_model_config(1)).ok(); | ||
| if let Some(client) = client { | ||
| let err = client.embed("not-a-real-model", &["x"]).await.unwrap_err(); | ||
| assert!(err.contains("unknown model alias"), "got: {}", err); | ||
| } | ||
| } |
There was a problem hiding this comment.
EmbeddingClient::new(small_model_config(1)) calls Self::load_model which downloads the AllMiniLML6V2 ONNX model — this is the same model loading path as the #[ignore]d tests. In a fresh CI environment (no cached model), construction fails and .ok() produces None, so the if let Some(client) block is never entered and the actual alias-rejection assertion never runs. The test passes vacuously every CI run, giving false assurance. The comment "No model loading on this path" is incorrect — the alias check lives in embed(), which is never reached if new() fails.
The cleanest fix is to test alias rejection through model::from_name directly (no client required), or add #[ignore] like the other e2e tests that need a real model.
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)