Set provider parameter when instantiating onnxruntime.InferenceSession #1976

cjb06776 · 2022-01-07T17:30:48Z

fixes #1973

Proposed changes:

Sets a default CUDAExecutionProvider or CPUExecutionProvider provider depending on the device type passed in, if no provider is specified.
Optionally allow passing in of a custom provider list
The providers parameter exists in earlier versions of the 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.

fixes deepset-ai#1973

julian-risch

There is an issue regarding type annotations. Please see my more detailed comment below. Otherwise looks good to me! 👍

julian-risch · 2022-01-10T09:26:00Z

haystack/modeling/model/adaptive_model.py

@@ -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"])


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.

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?

cjb06776 · 2022-01-14T19:17:53Z

@julian-risch I went ahead and just corrected the type annotation to use torch.device as that is what is currently being passed in by Inferencer.load: bfb21ab

julian-risch · 2022-01-25T08:12:13Z

Hi @cjb06776 sorry for letting you wait. I would like to check on the other points in the code where device is annotated as string rather than torch.device:

haystack/utils/augment_squad.py
modeling/evaluation/eval.py
modeling/model/adaptive_model.py
modeling/model/biadaptive_model.py
modeling/model/triadaptive_model.py
modeling/training/base.py
reader/farm.py

After that, I'll get back to you.

julian-risch · 2022-01-27T08:07:46Z

Hi @cjb06776 I hope you don't mind that I made a few changes here as well. I basically changed the type annotations of device to torch.device throughout the code base to give that a try. That way it's much more consistent. What I haven't done yet is to update also the doc strings. I would like to check with some of my colleagues before going forward. What's your opinion on the changes? Do the type annotations make more sense for you now? Thanks for bringing up this issue! 👍

cjb06776 · 2022-01-27T15:29:57Z

Makes sense to me. 👍

CLAassistant · 2022-01-27T18:10:28Z

All committers have signed the CLA.

ZanSara · 2022-03-16T18:06:44Z

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:

Merge master into your fork to avoid potential merge conflicts (we had a few large PRs lately) and
Complete what @julian-risch has started, so refactoring all uses of device: str and devices: List[str] to use torch.device as type instead.

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!

cjb06776 · 2022-03-18T19:41:46Z

Hi @ZanSara,

This PR already has Allow edits by maintainers selected as described in the link you reference.

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.

ZanSara · 2022-03-21T10:17:42Z

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

julian-risch

@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?

julian-risch

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.

haystack/utils/augment_squad.py

haystack/nodes/retriever/dense.py

haystack/nodes/reader/farm.py

haystack/modeling/conversion/transformers.py

…into cb-set-onnx-providers

julian-risch

LGTM! 👍

Set provider parameter when instantiating onnxruntime.InferenceSession

ede2708

fixes deepset-ai#1973

julian-risch self-requested a review January 10, 2022 08:59

julian-risch added the topic:speed label Jan 10, 2022

julian-risch requested changes Jan 10, 2022

View reviewed changes

Change device type to torch.device

bfb21ab

cjb06776 requested a review from julian-risch January 21, 2022 20:31

set type annotation of device to torch.device everywhere

6aa43ac

julian-risch mentioned this pull request Jan 31, 2022

Distribute intermediate layer distillation loss calculation over multiple GPUs #2090

Merged

4 tasks

ZanSara and others added 3 commits February 3, 2022 15:30

Merge branch 'master' into cb-set-onnx-providers

dc711da

Apply Black

32c8e0c

Change types of device and devices params across the codebase

0c41a96

Merge branch 'master' into cb-set-onnx-providers

009c7ef

github-actions bot and others added 6 commits March 21, 2022 10:19

Update Documentation & Code Style

26e031b

Add type: ignore in the right location

1421855

Merge branch 'cb-set-onnx-providers' of github.com:cjb06776/haystack …

bf3c231

…into cb-set-onnx-providers

Update Documentation & Code Style

8aa7e0d

Add type: ignore

93b284f

Merge branch 'cb-set-onnx-providers' of github.com:cjb06776/haystack …

6358741

…into cb-set-onnx-providers

ZanSara approved these changes Mar 21, 2022

View reviewed changes

julian-risch reviewed Mar 21, 2022

View reviewed changes

feedback

b8d87f3

ZanSara requested a review from julian-risch March 22, 2022 10:35

Update Documentation & Code Style

8f7a848

julian-risch requested changes Mar 22, 2022

View reviewed changes

julian-risch added the breaking change label Mar 22, 2022

ZanSara and others added 11 commits March 22, 2022 19:21

feedback 2

518f2c2

Fix convert_to_transformers

983b733

Merge branch 'cb-set-onnx-providers' of github.com:cjb06776/haystack …

890f904

…into cb-set-onnx-providers

Fix syntax error

8886c8e

Update Documentation & Code Style

7a45f51

Consider augment and load_glove user-facing as well

99b3137

Merge branch 'cb-set-onnx-providers' of github.com:cjb06776/haystack …

41b56ac

…into cb-set-onnx-providers

Update Documentation & Code Style

7d1a782

Fix mypy

49d3ec1

Merge branch 'cb-set-onnx-providers' of github.com:cjb06776/haystack …

a870152

…into cb-set-onnx-providers

Update Documentation & Code Style

c9b55e3

julian-risch approved these changes Mar 23, 2022

View reviewed changes

ZanSara merged commit 3b2001e into deepset-ai:master Mar 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set provider parameter when instantiating onnxruntime.InferenceSession #1976

Set provider parameter when instantiating onnxruntime.InferenceSession #1976

cjb06776 commented Jan 7, 2022

julian-risch left a comment

julian-risch Jan 10, 2022

cjb06776 Jan 10, 2022 •

edited

cjb06776 commented Jan 14, 2022

julian-risch commented Jan 25, 2022 •

edited

julian-risch commented Jan 27, 2022

cjb06776 commented Jan 27, 2022

CLAassistant commented Jan 27, 2022 •

edited

ZanSara commented Mar 16, 2022

cjb06776 commented Mar 18, 2022

ZanSara commented Mar 21, 2022

julian-risch left a comment

julian-risch left a comment

julian-risch left a comment

Set provider parameter when instantiating onnxruntime.InferenceSession #1976

Set provider parameter when instantiating onnxruntime.InferenceSession #1976

Conversation

cjb06776 commented Jan 7, 2022

julian-risch left a comment

Choose a reason for hiding this comment

julian-risch Jan 10, 2022

Choose a reason for hiding this comment

cjb06776 Jan 10, 2022 • edited

Choose a reason for hiding this comment

cjb06776 commented Jan 14, 2022

julian-risch commented Jan 25, 2022 • edited

julian-risch commented Jan 27, 2022

cjb06776 commented Jan 27, 2022

CLAassistant commented Jan 27, 2022 • edited

ZanSara commented Mar 16, 2022

cjb06776 commented Mar 18, 2022

ZanSara commented Mar 21, 2022

julian-risch left a comment

Choose a reason for hiding this comment

julian-risch left a comment

Choose a reason for hiding this comment

julian-risch left a comment

Choose a reason for hiding this comment

cjb06776 Jan 10, 2022 •

edited

julian-risch commented Jan 25, 2022 •

edited

CLAassistant commented Jan 27, 2022 •

edited