-
Notifications
You must be signed in to change notification settings - Fork 218
new: expose some onnx session options #578
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -64,6 +64,7 @@ def _load_onnx_model( | |||||||||
| providers: Optional[Sequence[OnnxProvider]] = None, | ||||||||||
| cuda: bool = False, | ||||||||||
| device_id: Optional[int] = None, | ||||||||||
| extra_session_options: Optional[dict[str, Any]] = None, | ||||||||||
| ) -> None: | ||||||||||
| super()._load_onnx_model( | ||||||||||
| model_dir=model_dir, | ||||||||||
|
|
@@ -72,6 +73,7 @@ def _load_onnx_model( | |||||||||
| providers=providers, | ||||||||||
| cuda=cuda, | ||||||||||
| device_id=device_id, | ||||||||||
| extra_session_options=extra_session_options, | ||||||||||
| ) | ||||||||||
| self.tokenizer, self.special_token_to_id = load_tokenizer(model_dir=model_dir) | ||||||||||
| assert self.tokenizer is not None | ||||||||||
|
|
@@ -122,6 +124,7 @@ def _embed_documents( | |||||||||
| device_ids: Optional[list[int]] = None, | ||||||||||
| local_files_only: bool = False, | ||||||||||
| specific_model_path: Optional[str] = None, | ||||||||||
| extra_session_options: Optional[dict[str, Any]] = None, | ||||||||||
| **kwargs: Any, | ||||||||||
| ) -> Iterable[T]: | ||||||||||
| is_small = False | ||||||||||
|
|
@@ -153,6 +156,9 @@ def _embed_documents( | |||||||||
| **kwargs, | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if extra_session_options is not None: | ||||||||||
| params.update(extra_session_options) | ||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||||||
|
|
||||||||||
| pool = ParallelWorkerPool( | ||||||||||
| num_workers=parallel or 1, | ||||||||||
| worker=self._get_text_worker_class(), | ||||||||||
|
|
@@ -189,6 +195,7 @@ def _embed_images( | |||||||||
| device_ids: Optional[list[int]] = None, | ||||||||||
| local_files_only: bool = False, | ||||||||||
| specific_model_path: Optional[str] = None, | ||||||||||
| extra_session_options: Optional[dict[str, Any]] = None, | ||||||||||
| **kwargs: Any, | ||||||||||
| ) -> Iterable[T]: | ||||||||||
| is_small = False | ||||||||||
|
|
@@ -220,6 +227,9 @@ def _embed_images( | |||||||||
| **kwargs, | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if extra_session_options is not None: | ||||||||||
| params.update(extra_session_options) | ||||||||||
|
Comment on lines
+230
to
+231
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical bug: session options are incorrectly flattened into params. Same issue as in Apply this diff: - if extra_session_options is not None:
- params.update(extra_session_options)
+ if extra_session_options is not None:
+ params["extra_session_options"] = extra_session_options📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
|
|
||||||||||
| pool = ParallelWorkerPool( | ||||||||||
| num_workers=parallel or 1, | ||||||||||
| worker=self._get_image_worker_class(), | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,6 +117,8 @@ def __init__( | |
| self.device_ids = device_ids | ||
| self.cuda = cuda | ||
| self.device_id = device_id | ||
| self._extra_session_options = self._select_exposed_session_options(kwargs) | ||
|
|
||
|
Comment on lines
+120
to
+121
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extra session options initialized but not propagated. The Apply this diff to propagate the session options: def load_onnx_model(self) -> None:
self._load_onnx_model(
model_dir=self._model_dir,
model_file=self.model_description.model_file,
threads=self.threads,
providers=self.providers,
cuda=self.cuda,
device_id=self.device_id,
+ extra_session_options=self._extra_session_options,
)Also add to the yield from self._embed_documents(
model_name=self.model_name,
cache_dir=str(self.cache_dir),
documents=documents,
batch_size=batch_size,
parallel=parallel,
providers=self.providers,
cuda=self.cuda,
device_ids=self.device_ids,
k=self.k,
b=self.b,
avg_len=self.avg_len,
is_query=False,
local_files_only=self._local_files_only,
specific_model_path=self._specific_model_path,
+ extra_session_options=self._extra_session_options,
**kwargs,
)And similarly for
|
||
| self.k = k | ||
| self.b = b | ||
| self.avg_len = avg_len | ||
|
|
@@ -153,6 +155,7 @@ def load_onnx_model(self) -> None: | |
| providers=self.providers, | ||
| cuda=self.cuda, | ||
| device_id=self.device_id, | ||
| extra_session_options=self._extra_session_options, | ||
| ) | ||
|
|
||
| assert self.tokenizer is not None | ||
|
|
@@ -221,6 +224,7 @@ def embed( | |
| is_query=False, | ||
| local_files_only=self._local_files_only, | ||
| specific_model_path=self._specific_model_path, | ||
| extra_session_options=self._extra_session_options, | ||
| **kwargs, | ||
| ) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't expect this to be expanded at runtime it could be an enum likeor similar, would make type hinting nicer and still allowsinchecks and iteration.Nvm,
StrEnumwas added with 3.11.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it can be emulated with
ExposedSessionOptions(str, Enum), but feels like a bit too much, users won't see this enum anywayThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's not worth it to emulate it that way, it was a low hanging fruit with
StrEnumbuilt in but definitely not worth reimplementing it (also just(str, Enum)won't give youautoiirc so it'd be even more work). I just misremembered whenStrEnumwas added.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could type it as a
tuple[Literal[...]]but again, not worth the effort for something that's only internal.