fix: disable transformers progress bar on Windows#2368
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA private helper ChangesTransformers progress bar disable to fix Windows OSError
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🧹 Nitpick comments (3)
core/wren/src/wren/memory/embeddings.py (3)
28-29: 💤 Low valueHard-coded model assumptions in docstring are unverified.
The docstring claims the default model is "all-MiniLM-L6-v2" with "1024-dimensional" embeddings, but there's no code to verify or enforce this. The actual model and dimension depend on LanceDB's registry defaults, which could change between versions or be configured differently.
Remove the hard-coded model name and dimension from the docstring, or query them from the created embedding function to ensure accuracy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/wren/src/wren/memory/embeddings.py` around lines 28 - 29, The docstring in core/wren/src/wren/memory/embeddings.py hardcodes a model name and embedding dimension; remove those specific claims or make them dynamic by querying the actual embedding function/instance at runtime. Update the module-level docstring (and any references in functions or classes that mention the model or dimension) to either omit the specific model/dimension or retrieve them from the created embedding function (e.g., from the embedding factory/instance used when creating embeddings) and include that information only after verifying its attributes. Ensure you reference and adjust any code that assumes "all-MiniLM-L6-v2" or "1024-dimensional" so behavior matches the actual embedding provider.
38-38: ⚡ Quick winRedundant
get_registry()call.The registry is already obtained at module import time (line 14) and stored in the module-level
registryvariable. Callingget_registry()again is unnecessary. Reuse the module-level registry or cache it.♻️ Proposed fix to reuse module-level registry
def get_embedding_function(): """ Return a LanceDB-compatible embedding function for sentence-transformers. Returns: A callable that accepts a batch of strings and returns a list of embeddings. - Each embedding is a list of floats (1024-dimensional for the default - all-MiniLM-L6-v2 model). Raises: ImportError: If sentence-transformers or lancedb is not installed. """ if not _SENTENCE_TRANSFORMERS_AVAILABLE: raise ImportError( "sentence-transformers not available. Install with: pip install 'wrenai[memory]'" ) - registry = get_registry() return registry.get("sentence-transformers").create()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/wren/src/wren/memory/embeddings.py` at line 38, The duplicate call to get_registry() (the local "registry = get_registry()" at the shown location) is redundant because a module-level registry variable is already created at import time; remove the extra call and reuse the module-level registry variable instead (ensure you don't shadow it with a new local assignment in functions or classes such as any function referencing registry in this file), keeping references to registry as-is so the module-level cached instance is used.
39-39: ⚖️ Poor tradeoffNo way to configure which sentence-transformers model to use.
The old API accepted a
model_nameparameter to select different models (e.g., multilingual vs. English). The new API callsregistry.get("sentence-transformers").create()with no arguments, using only LanceDB's default model. This removes the ability to customize the embedding model, which may be needed for different languages or performance requirements.Consider adding back model selection capability, either as a function parameter or through the registry create method.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/wren/src/wren/memory/embeddings.py` at line 39, The current call to registry.get("sentence-transformers").create() in embeddings.py removes the ability to pick a specific sentence-transformers model; update the embeddings factory to accept a model_name parameter and pass it through to the registry creator (e.g., change the function that returns registry.get("sentence-transformers").create() to accept model_name and call registry.get("sentence-transformers").create(model_name=model_name) or similar), or alternatively update the registry implementation to support create(model_name=...) so callers can select multilingual/English models via the model_name argument; ensure the parameter name (model_name) is exposed in the public function signature that currently returns registry.get("sentence-transformers").create() so existing call sites can opt into custom models.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/wren/src/wren/memory/embeddings.py`:
- Around line 34-39: The change removed the ability to obtain the vector
dimension needed by Arrow schema creation (formerly via _DEFAULT_DIM/warm_up),
so update the sentence-transformers embedding API to expose dimension
information: modify the code path that returns
registry.get("sentence-transformers").create() (embedding factory) so the
created object exposes a stable dimension attribute or method (e.g.,
embedding.dim or embedding.get_dimension()) or return a tuple (embedding, dim);
ensure callers like store.py can call that attribute/method (or unpack the
tuple) to obtain the integer used for pa.list_(pa.float32(), dim).
- Around line 13-19: The current bare except around get_registry() masks all
errors; update the block that sets _SENTENCE_TRANSFORMERS_AVAILABLE (the call to
get_registry() and the membership check for "sentence-transformers") to only
catch the expected exceptions (e.g., ImportError, KeyError, possibly
RuntimeError) that indicate the embedding backend or package is unavailable, and
let other exceptions bubble up (or log and re-raise) so real bugs in
get_registry() are not silently swallowed; reference the get_registry() call and
the _SENTENCE_TRANSFORMERS_AVAILABLE assignment when making this change.
- Line 22: The embeddings module must match the MemoryStore API: change
get_embedding_function to accept a model_name parameter (def
get_embedding_function(model_name=None)), export _DEFAULT_MODEL, _DEFAULT_DIM,
and warm_up, and ensure _SENTENCE_TRANSFORMERS_AVAILABLE remains defined; update
the function to use a single get_registry() call (remove the duplicate call at
module import or inside the function) and compute or set _DEFAULT_DIM
dynamically rather than hardcoding "1024" in the docstring—either derive the
dimension from the resolved model metadata or remove the specific dimension
text; ensure imports in store.py (get_embedding_function, _DEFAULT_MODEL,
_DEFAULT_DIM, warm_up) succeed with these symbols exported.
- Around line 5-7: The module-level call to
transformers_logging.disable_progress_bar() in wren.memory.embeddings causes a
global side effect; remove that import-time call and instead call
disable_progress_bar() only around the specific code paths that instantiate or
warm up the transformer embedding model (e.g., inside the warm_up function or
the embedding model creation function/class in wren.memory.embeddings),
optionally guarded by an opt-out config/env flag, and restore or avoid disabling
globally afterward (implement a small helper or context-like toggle around model
creation to localize the effect rather than calling
transformers_logging.disable_progress_bar() at import time).
---
Nitpick comments:
In `@core/wren/src/wren/memory/embeddings.py`:
- Around line 28-29: The docstring in core/wren/src/wren/memory/embeddings.py
hardcodes a model name and embedding dimension; remove those specific claims or
make them dynamic by querying the actual embedding function/instance at runtime.
Update the module-level docstring (and any references in functions or classes
that mention the model or dimension) to either omit the specific model/dimension
or retrieve them from the created embedding function (e.g., from the embedding
factory/instance used when creating embeddings) and include that information
only after verifying its attributes. Ensure you reference and adjust any code
that assumes "all-MiniLM-L6-v2" or "1024-dimensional" so behavior matches the
actual embedding provider.
- Line 38: The duplicate call to get_registry() (the local "registry =
get_registry()" at the shown location) is redundant because a module-level
registry variable is already created at import time; remove the extra call and
reuse the module-level registry variable instead (ensure you don't shadow it
with a new local assignment in functions or classes such as any function
referencing registry in this file), keeping references to registry as-is so the
module-level cached instance is used.
- Line 39: The current call to registry.get("sentence-transformers").create() in
embeddings.py removes the ability to pick a specific sentence-transformers
model; update the embeddings factory to accept a model_name parameter and pass
it through to the registry creator (e.g., change the function that returns
registry.get("sentence-transformers").create() to accept model_name and call
registry.get("sentence-transformers").create(model_name=model_name) or similar),
or alternatively update the registry implementation to support
create(model_name=...) so callers can select multilingual/English models via the
model_name argument; ensure the parameter name (model_name) is exposed in the
public function signature that currently returns
registry.get("sentence-transformers").create() so existing call sites can opt
into custom models.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 54678a6b-c6bb-4001-8fe5-3dc309dcca6f
📒 Files selected for processing (1)
core/wren/src/wren/memory/embeddings.py
goldmedal
changed the title
|
Hi @Federicorao, thanks for you working on this. There are some CI fails. Could you fix them? |
Federicorao
changed the title
transformers only ships with the optional memory extra, so importing transformers.utils.logging at module top level makes wren.memory.embeddings unimportable when that extra is not installed. Move the import into _disable_transformers_progress_bar (lazy, matching the existing lazy lancedb import) so the module stays importable without the extra.
|
Thanks @goldmedal — went back through the change and fixed a real issue: the new I moved it into The only check that ran here is |
goldmedal
left a comment
goldmedal
left a comment
There was a problem hiding this comment.
Thanks @Federicorao 👍
2 participants
Fixes #2366.
Disable transformers progress bar before loading sentence-transformers to prevent OSError on Windows due to sys.stderr.flush() failure. On Windows, the tqdm progress bar used by transformers/huggingface internally calls sys.stderr.flush() which can raise OSError: [WinError 1] Incorrect function in certain terminals (e.g., Git Bash). Calling transformers.logging.disable_progress_bar() before importing sentence-transformers or lancedb.embeddings eliminates this crash without changing any other behavior.
I checked that the changed Python files still parse correctly.
Summary by CodeRabbit