-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Set provider parameter when instantiating onnxruntime.InferenceSession #1976
Conversation
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.
There is an issue regarding type annotations. Please see my more detailed comment below. Otherwise looks good to me! 👍
@@ -637,7 +637,9 @@ def load(cls, load_dir: Union[str, Path], device: str, **kwargs): # type: ignor | |||
sess_options.graph_optimization_level = onnxruntime.GraphOptimizationLevel.ORT_ENABLE_EXTENDED | |||
# Use OpenMP optimizations. Only useful for CPU, has little impact for GPUs. | |||
sess_options.intra_op_num_threads = multiprocessing.cpu_count() | |||
onnx_session = onnxruntime.InferenceSession(str(load_dir / "model.onnx"), sess_options) | |||
|
|||
providers = kwargs.get("providers", ["CPUExecutionProvider"] if device.type == "cpu" else ["CUDAExecutionProvider"]) |
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.
Just a small thing that also came up in the tests run by the CI:
haystack/modeling/model/adaptive_model.py:641: error: "str" has no attribute "type"
The type annotations of the load
method suggest that device
is of type str
and therefore has no .type
attribute. Could you maybe test with comparing directly to the string, like device == "cpu"
? Otherwise looks good to me.
An alternative to else ["CUDAExecutionProvider"])
would have been to list multiple providers sorted by priority, e.g., ['CUDAExecutionProvider', 'CPUExecutionProvider']
but I think it's better to make it explicit here and let it fail otherwise.
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.
The type annotation on the method is either incorrect or this is an existing problem with other callers of this method...
What ends up passed in is a torch.device
. I don't know if this intended, and/or a bug, but Inferencer.load
passes in a torch.device
when calling this method. (which makes AdaptiveModel.load
type annotations similarly incorrect).
So there may be some larger issues here that need to be cleaned up/fixed beyond the scope of this small PR. Let me know how you want to proceed here to resolve the CI error.... I could change the type annotation to be a torch.device
just in ONNXAdapativeModel
. Or if that is not desired, would you want to suppress/ignore the type check on this line?
@julian-risch I went ahead and just corrected the type annotation to use |
Hi @cjb06776 sorry for letting you wait. I would like to check on the other points in the code where
After that, I'll get back to you. |
Hi @cjb06776 I hope you don't mind that I made a few changes here as well. I basically changed the type annotations of |
Makes sense to me. 👍 |
Hello @cjb06776, sorry for the long silence. I've decided to pick up this PR and merge it to Haystack, but to do so I'd need to do a couple of things:
I've actually cloned your fork and performed these changes already, but I am now unable to push them upstream. Could you check that you allowed us to push to your fork? If you're unsure how to check, there are some instructions here: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork Thanks a lot! |
Hi @ZanSara, This PR already has Additionally I've added you as a collaborator to my fork, hopefully that will resolve the access restriction you ran into. Let me know if that works or you need me to check something else to enable to allow you to proceed. |
Hey @cjb06776, great! I can push now. I took the liberty of merging the latest master back to your fork, to avoid complicated merge conflicts in here. If the CI is green I'll proceed to merge to master in the next few hours. Thank you again for your contribution. Much appreciated! |
…into cb-set-onnx-providers
…into cb-set-onnx-providers
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.
@ZanSara The typing is definitely much cleaner now, thanks! However, I thought we agreed that user-facing functions should still allow setting the device parameter as a str "cpu" or "gpu", no? For example, the initialization of ranker or retriever nodes would be easier for users if device can be a string. What do you think?
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.
augment_squad.py
needs to be checked again but all other changes looks good to me. The docstrings for the methods that allow device
to be either str
or torch.device
should also mention the former option.
I could imagine that some users are using the Inferencer directly because it was the way to use FARM before the pipeline concept. In that sense, this PR is a breaking change for some users so we should mention that in the release notes.
…into cb-set-onnx-providers
…into cb-set-onnx-providers
…into cb-set-onnx-providers
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.
LGTM! 👍
fixes #1973
Proposed changes:
onnxruntime.InferenceSession
constructor (checked back as far as 1.4, it just wasn't required), so I don't believe there should be any backward compatibility issues.