Skip to content
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

update instructor for sentence transformers #129

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

BBC-Esq
Copy link
Contributor

@BBC-Esq BBC-Esq commented Jan 14, 2025

Necessary because sentence transformers is now using the "backend" parameter when instantiating a model and the passing of "module_kwargs = {}" due to recent changes with sentence transformers.

This differs from the following two other pull requests that attempt to address the same issue, as outlined below...but the core unique feature of mine is that it's backward compatible with older versions of the sentence-transformers library:

Aspect My Approach PR #128 PR #127
Backend Parameter Implementation Uses inspect.signature() to check if backend parameter exists at runtime Uses Literal type for strict type checking: backend: Literal['torch', 'onnx', 'openvino'] = 'torch' Uses simple string parameter: backend: str = "torch"
Parameter Passing Adds backend to model_args if detected Passes backend directly in constructor signature Passes backend as separate positional argument to _load_model
Type Safety Medium - Runtime checking but no static type hints High - Uses Python type hints to enforce valid values Low - Simple string type without validation
Backward Compatibility High - Only uses backend parameter if supported Low - Requires backend parameter Medium - Always requires backend but without strict validation

refactor: few updates to have the functionality on-par with sentence-transformers and other minor updates
Necessary because sentence transformers is now using the "backend" parameter when instantiating a model and the passing of "module_kwargs = {}" due to recent changes with sentence transformers.
Forgot to make whether to return "module_kwargs = {}" also conditional on if a user is using an older or newer version of sentence transformers.
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.

1 participant