feat: make @huggingface/transformers optional and improve error handling in EmbeddingService #7
Conversation
…ing in EmbeddingService
WalkthroughTransformer support was converted to an optional peer dependency and removed from top-level dependencies; EmbeddingService now lazy-loads/injects the transformers module with an explicit initialize lifecycle and dimension probing; docs and ESLint were updated to note/allow optional loading; CI gained a retry build step. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EmbeddingService
participant TransformersModule
participant Filesystem
Client->>EmbeddingService: call initialize() or embed(text)
alt not initialized
EmbeddingService->>TransformersModule: dynamic import / injected module (loadTransformers)
TransformersModule-->>EmbeddingService: return pipeline factory
EmbeddingService->>Filesystem: optional cache/config setup
EmbeddingService->>TransformersModule: create pipeline and run probe embedding
TransformersModule-->>EmbeddingService: probe embeddings (used to set dimensions)
EmbeddingService-->>Client: initialization complete (or throw ConfigurationError)
else initialized
Client->>EmbeddingService: embed(text)
EmbeddingService->>TransformersModule: run pipeline
TransformersModule-->>EmbeddingService: embeddings
EmbeddingService-->>Client: return embeddings
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/vectoriadb/src/embedding.service.ts (1)
15-15: Consider adding type information for the pipeline.While
anyis pragmatic for the dynamically loaded pipeline, you could improve type safety slightly by typing it as the return type of the pipeline function:private pipeline: Awaited<ReturnType<PipelineFunction>> | null = null;This is a minor refinement and the current implementation works correctly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/enclave-vm/package.json(0 hunks)libs/vectoriadb/package.json(1 hunks)libs/vectoriadb/src/embedding.service.ts(4 hunks)package.json(0 hunks)
💤 Files with no reviewable changes (2)
- package.json
- libs/enclave-vm/package.json
🧰 Additional context used
📓 Path-based instructions (1)
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/vectoriadb/src/embedding.service.tslibs/vectoriadb/package.json
🔇 Additional comments (5)
libs/vectoriadb/src/embedding.service.ts (5)
1-4: LGTM! Proper type definition for dynamic imports.The
PipelineFunctiontype alias usingtypeof import(...)is the correct TypeScript pattern for typing dynamically imported functions. TheConfigurationErrorimport appropriately supports the new error handling flow.
8-12: LGTM! Clear documentation for optional dependency.The updated documentation clearly communicates the optional nature of the dependency and provides both installation instructions and an alternative solution.
27-42: LGTM! Proper dynamic import with clear error guidance.The
loadTransformersmethod correctly implements dynamic loading with appropriate error handling. TheConfigurationErrorprovides actionable guidance for users, directing them to either install the package or use the zero-dependency alternative.
63-67: LGTM! Correct implementation of lazy loading.The initialization properly loads the transformers dynamically before creating the pipeline, successfully implementing the lazy loading pattern while maintaining the existing pipeline configuration.
84-86: LGTM! Proper error type preservation.The error handling correctly distinguishes between
ConfigurationError(which should bubble up with its specific message) and other errors (which get wrapped inEmbeddingError). This prevents double-wrapping while maintaining clear error semantics.
…efine pipeline type
…ity with complex return types
…ity with complex return types
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/vectoriadb/src/embedding.service.ts (1)
15-17: Consider typing the pipeline more precisely.While the current
anytype with explanation is acceptable, you could potentially type it asAwaited<ReturnType<PipelineFunction>>or create a more specific type alias for better type safety. However, given the complexity of the transformer types, the current approach with documentation is reasonable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/draft/docs/libraries/vectoriadb.mdx(1 hunks)docs/live/docs/libraries/vectoriadb.mdx(1 hunks)libs/enclave-vm/package.json(1 hunks)libs/vectoriadb/package.json(1 hunks)libs/vectoriadb/src/embedding.service.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/vectoriadb/package.json
🧰 Additional context used
📓 Path-based instructions (3)
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/vectoriadb/src/embedding.service.tslibs/enclave-vm/package.json
docs/draft/docs/**
⚙️ CodeRabbit configuration file
docs/draft/docs/**: This folder holds the draft/source docs that humans are expected to edit. When authors want to add or change documentation, they should do it here. The Codex workflow uses these drafts, together with the code diff, to generate the latest docs under docs/docs/. As a reviewer: - Encourage contributors to add/update content here instead of docs/docs/. - It is fine to do structural/content feedback here (clarity, examples, etc).
Files:
docs/draft/docs/libraries/vectoriadb.mdx
docs/**
⚙️ CodeRabbit configuration file
docs/**: Repository documentation for the SDK, using MDX and hosted by Mintlify. See more specific rules for: - docs/docs/** (latest rendered docs, automation-only) - docs/v/** (archived versions, read-only) - docs/draft/docs/** (human-editable drafts) - docs/blogs/** (blogs, human edited) - docs/docs.json (Mintlify navigation)
Files:
docs/draft/docs/libraries/vectoriadb.mdxdocs/live/docs/libraries/vectoriadb.mdx
🔇 Additional comments (7)
libs/vectoriadb/src/embedding.service.ts (4)
1-4: LGTM! Clean type-safe dynamic import pattern.The addition of ConfigurationError and the PipelineFunction type alias using
typeof import()provides proper type safety for the dynamic loading pattern while maintaining the optional dependency model.
6-13: LGTM! Clear documentation of optional dependency.The JSDoc comment clearly communicates the optional nature of the transformer dependency, provides installation instructions, and references the zero-dependency alternative.
29-44: LGTM! Clean dynamic import with helpful error message.The
loadTransformers()method correctly implements lazy loading with a clear error message. The catch-all approach treats any import failure as a missing dependency, which provides a simple user experience. If needed in the future, you could differentiate between MODULE_NOT_FOUND errors and other import failures for more precise diagnostics.
63-94: LGTM! Well-structured initialization with proper error handling.The refactored
_initialize()method correctly:
- Dynamically loads transformers via
loadTransformers()- Uses the loaded pipeline function to create the pipeline
- Preserves
ConfigurationErrorfor missing dependencies- Wraps other errors in
EmbeddingErrorfor consistent error handlingThe error handling strategy appropriately distinguishes between configuration issues (missing dependency) and runtime failures.
docs/live/docs/libraries/vectoriadb.mdx (1)
56-60: Verify documentation workflow - potential direct edit of rendered docs.According to the coding guidelines,
docs/docs/**(and possiblydocs/live/docs/**) should be automation-only and not directly edited by humans. Changes should be made indocs/draft/docs/**and then generated into the live docs. Since this identical change appears in bothdocs/live/docs/**anddocs/draft/docs/**, verify this follows the correct Codex workflow.Based on coding guidelines, contributors should edit
docs/draft/docs/**only, and the automation generatesdocs/docs/**.docs/draft/docs/libraries/vectoriadb.mdx (1)
56-60: LGTM! Clear documentation in the correct location.The note appropriately documents the optional nature of the transformer dependency with clear installation instructions. This change is correctly placed in
docs/draft/docs/**, which is the human-editable documentation source per the coding guidelines.libs/enclave-vm/package.json (1)
46-46: Version @huggingface/transformers@3.2.2 exists and is valid.The version is published in npm registry and the update from ^3.0.0 to ^3.2.2 involves only minor version increments (no major version change), which do not introduce breaking changes per semantic versioning. However, note that version 3.8.1 is now available; consider evaluating whether a more recent minor version would be appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/vectoriadb/src/embedding.service.ts (1)
16-18: Clarify/init-guardgetDimensionsto avoid stale values for non-default models
dimensionsis initialized to384(the default MiniLM size) and only updated after_initializeruns and the test embedding returns.getDimensions()simply returns the current value without checkingisInitialized.If a caller customizes
modelNameto something with a different embedding size and then callsgetDimensions()before callinginitialize()(or before any embedding has been generated), they’ll get384even though the eventual model might produce a different dimension. That can lead to subtle bugs when pre-sizing indexes or schemas fromgetDimensions().Two possible directions:
- Enforce initialization: have
getDimensions()throw (or at least log) if!this.isInitialized, so callers are forced toawait initialize()first.- Make the default more obviously “unknown”: e.g., use
private dimensions: number | null = null;and either throw or document returningnulluntil initialized (this would be a breaking change).Given this is a library API, I’d at least guard
getDimensions()with anisInitializedcheck and document the requirement to callinitialize()when using non-default models.Also applies to: 152-171
🧹 Nitpick comments (3)
libs/enclave-vm/src/scoring/scorers/local-llm.scorer.ts (1)
104-135: Dynamic import viaFunctionis brittle invmand matches the CI warning; consider a simpler pattern and clearer optional-dep handlingThe new pattern
// Dynamic import using Function to avoid TypeScript checking for the optional dependency // eslint-disable-next-line @typescript-eslint/no-explicit-any const transformers = await (Function('return import("@huggingface/transformers")')() as Promise<any>); const { pipeline } = transformers;is quite non‑obvious and appears to be what’s triggering the CI warning:
LocalLlmScorer: Model load failed, using rule-based fallback: TypeError [ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG]: A dynamic import callback was invoked without --experimental-vm-modules
inside the enclave VM. That means in vm contexts you’re effectively always on the rule‑based fallback, and the local‑LLM path never initializes.
A few concrete suggestions:
Prefer a straightforward dynamic import with TS suppression over
Function
It’s generally easier to maintain something like this for an optional peer:// Lazy‑load transformers as an optional peer dependency. // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-expect-error: @huggingface/transformers is an optional peer dependency and may not be installed. const { pipeline } = await import('@huggingface/transformers');This keeps the runtime behavior obvious (plain
import()), avoids theFunctionindirection, and still lets you mark the dependency as optional at the type‑level.Differentiate “not installed” from other failures in the catch block
To better match the PR goal (“optional” transformers), it would help to special‑case module‑not‑found (and possibly the vm dynamic‑import error) so the log is more actionable, e.g.:} catch (error) { this.initPromise = null;
- if (this.fallbackScorer) {
console.warn(`[LocalLlmScorer] Model load failed, using rule-based fallback: ${error instanceof Error ? error.message : String(error)}`,);
- if (this.fallbackScorer) {
const message = error instanceof Error ? error.message : String(error);const code = (error as { code?: string }).code;const prefix =code === 'ERR_MODULE_NOT_FOUND'? '[LocalLlmScorer] @huggingface/transformers not installed; using rule-based fallback': '[LocalLlmScorer] Model load failed; using rule-based fallback';console.warn(`${prefix}: ${message}`); this.ready = true; // Ready with fallback
throw new LocalLlmScorerError(
Failed to initialize model: ${error instanceof Error ? error.message : String(error)},
);
}
}This makes it clear when the optional dependency simply isn’t present vs. when the environment (e.g., vm without `--experimental-vm-modules`) is blocking dynamic imports.
- Verify vm behavior is what you want in production
Given the CI warning, it’d be good to double‑check that in your real enclave runtime you either:
- start Node with
--experimental-vm-modules(or equivalent support for vm dynamic imports), or- are intentionally okay with LocalLlmScorer always falling back to rule‑based scoring when running inside the vm.
If you confirm that dynamic
import()inside the enclave vm will never be supported, a more robust long‑term design might be to load@huggingface/transformersin the host process and inject a ready‑madepipelineinto the vm rather than importing from within the vm sandbox.libs/vectoriadb/src/embedding.service.ts (2)
25-41: Improve diagnostics in dynamic import error handlingThe dynamic import pattern for the optional
@huggingface/transformerslooks good, and rethrowingConfigurationErrorseparately from other failures in_initializeis a nice touch.Right now, any failure in
loadTransformers(including non–module-not-found issues like bundler/runtime config problems) is surfaced as “is not installed”, and the original error is dropped. That can make debugging harder when the package is actually present but misconfigured.Consider including the original error message (or at least its
.message) in theConfigurationError, or narrowing the catch to truly “module not found” cases and letting other errors bubble asEmbeddingErrorfrom_initialize.Example tweak:
} catch (error) { const details = error instanceof Error ? ` Details: ${error.message}` : ''; throw new ConfigurationError( '@huggingface/transformers could not be loaded. ' + 'Install it with: npm install @huggingface/transformers\n' + 'Or use TFIDFEmbeddingService for a zero-dependency alternative.' + details, ); }This preserves the optional-dependency behavior while improving observability for misconfigurations.
Also applies to: 62-67, 81-90
1-10: Align messaging and docs around the TFIDF alternative and optional dependencyThe top-level JSDoc and the
ConfigurationErrormessage both point users toward a TFIDF-based alternative, which is great for the “optional transformers” story. A couple of small nits:
- The error string references
"TFIDFVectoria/TFIDFEmbeddingService", while the doc comment just mentionsTFIDFEmbeddingService. If the actual export/path is different, this may confuse users. It’s worth standardizing the exact name/path across both.- The doc comment says “This service requires @huggingface/transformers to be installed.” That’s true for using this particular service, but in the context of making the dependency optional, you might want to phrase it as “This service depends on @huggingface/transformers; for a zero-dependency alternative, use …”. That makes the optional nature of the package clearer.
Also, since this class’s public API has evolved (new
initialize,getDimensions,isReady, and dynamic dimension probing), ensure the corresponding docs underdocs/draft/docs/**for the vectoriadb embedding service are updated to describe the new lifecycle and the optional dependency behavior.Also applies to: 35-39
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/enclave-vm/eslint.config.mjs(1 hunks)libs/enclave-vm/src/scoring/scorers/local-llm.scorer.ts(1 hunks)libs/vectoriadb/eslint.config.mjs(1 hunks)libs/vectoriadb/src/embedding.service.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/vectoriadb/eslint.config.mjslibs/vectoriadb/src/embedding.service.tslibs/enclave-vm/src/scoring/scorers/local-llm.scorer.tslibs/enclave-vm/eslint.config.mjs
🪛 GitHub Actions: CI
libs/enclave-vm/src/scoring/scorers/local-llm.scorer.ts
[warning] 124-124: LocalLlmScorer: Model load failed, using rule-based fallback: TypeError [ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG]: A dynamic import callback was invoked without --experimental-vm-modules
🔇 Additional comments (2)
libs/vectoriadb/eslint.config.mjs (1)
21-28: ESLint dependency-checks ignore entry looks correctMarking
@huggingface/transformersas an ignored dependency here matches the new optional/dynamic loading pattern and avoids false-positive NX dependency errors. No issues from a tooling or runtime perspective.libs/enclave-vm/eslint.config.mjs (1)
6-13: Consistent handling of optional transformers dependency in ESLintAdding
ignoredDependencies: ['@huggingface/transformers']here keeps NX dependency checks aligned with the optional/dynamic-import usage and mirrors the vectoriadb config. This looks consistent and safe.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/vectoriadb/src/embedding.service.ts (1)
54-66: Consider using direct dynamic import instead of Function() wrapper.The
Function()wrapper bypasses static analysis but is unconventional and may cause bundler issues. Modern TypeScript and bundlers support dynamic imports directly.Consider this simpler approach:
try { - // Dynamic import - package may not be installed - // Using Function() to bypass TypeScript's static analysis for optional dependency - const transformers = await (Function('return import("@huggingface/transformers")')() as Promise<any>); + // Dynamic import - package may not be installed + const transformers = await import('@huggingface/transformers'); return transformers.pipeline; } catch (_error) {If TypeScript complains about the missing module, you can add a
// @ts-expect-errorcomment or declare the module type separately. This approach is more standard and bundler-friendly.If you keep the Function() approach, verify compatibility with your bundler (Webpack, Vite, esbuild, etc.) to ensure the dynamic import is preserved correctly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/vectoriadb/jest.setup.ts(1 hunks)libs/vectoriadb/src/embedding.service.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/vectoriadb/jest.setup.tslibs/vectoriadb/src/embedding.service.ts
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (8)
libs/vectoriadb/jest.setup.ts (4)
24-26: Verify array handling in mock pipeline.The parameter accepts
string | string[], buttoString()on an array produces comma-separated values (e.g.,['hello', 'world']becomes'hello,world'). This could produce unexpected embeddings if arrays are passed.If arrays are never passed in practice, consider narrowing the type to
stringfor clarity. Otherwise, handle arrays explicitly:- return async (text: string | string[]) => { - const textStr = text.toString(); + return async (text: string | string[]) => { + const textStr = Array.isArray(text) ? text.join(' ') : text;
8-21: LGTM!The helper functions for word extraction and normalization are well-implemented for test purposes. The simple stemming approach is appropriate for generating consistent mock embeddings.
30-66: LGTM!The embedding generation logic is sophisticated and realistic. The approach of using word hashing with Gaussian contributions ensures that texts with overlapping words have high similarity, which accurately simulates transformer behavior for testing purposes.
74-89: LGTM!The mock module and lifecycle hooks are correctly implemented. The runtime injection approach via
setTransformersModuleandclearTransformersModuleprovides better control than static jest.mock and aligns with the new optional dependency pattern.libs/vectoriadb/src/embedding.service.ts (4)
3-10: LGTM!The updated documentation clearly communicates that
@huggingface/transformersis optional and provides actionable guidance for installation or alternatives.
12-29: LGTM!The testing hooks are well-designed and appropriately marked as
@internal. This pattern enables effective testing without exposing test-specific APIs to consumers.
85-116: LGTM!The initialization logic correctly integrates dynamic loading and distinguishes between configuration errors (missing dependency) and runtime errors (initialization failures). The error handling appropriately propagates
ConfigurationErrorunchanged while wrapping other errors with context.
1-197: Verify documentation updates for API changes.This PR makes
@huggingface/transformersoptional, which affects the public API behavior. Per the coding guidelines forlibs/**, when public APIs change, ensure there is a matchingdocs/draft/docs/**update.Please confirm that the documentation has been updated to reflect:
- The optional nature of
@huggingface/transformers- Installation instructions for the optional dependency
- Guidance on when to use
TFIDFEmbeddingServiceas an alternative- Any migration notes for users upgrading from previous versions
Based on coding guidelines: publishable SDK libraries should have matching documentation for API changes.
Summary by CodeRabbit
Chores
New Behavior
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.