Adding the new device API, fixing the a nested dict issue in the existing compile phase, adding new lowering pass for bn #288

narendasan · 2021-01-22T04:40:20Z

Description

This PR adds a new device sub struct to the CompileSpec structure reflecting the new DLA changes.
It also addresses a nested dictionary issue in the compile phase of the backend api.
Finally adds a lowering pass that addresses new dimension checks for the batchnorm operator.

This PR also bumps bazel and TensorRT versions in preparation for the next release.

Fixes #286

Type of change

Please delete options that are not relevant and/or add your own.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the style guidelines of this project (You can use the linters)
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas and hacks
I have made corresponding changes to the documentation
I have added tests to verify my fix or my feature
New and existing unit tests pass locally with my changes

to_backend Also fixes nested dictionary bug reported in #286 Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>

batchnorm Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>

BREAKING CHANGE: Version of bazel has been bumped to 4.0.0 Version of TensorRT has been bumped to 7.2.2.3 Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>

Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]> docs: Update docs for to_backend API for new device API and new PyTorch API Changes the docs to show the new device dictionary API and how to use the new to backend api (changed from PyTorch 1.6.0) Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>

Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

andi4191

It seems to be a combination of 3 PRs
a) TensorRT and bazel version bumps
b) Batch normalization dimension check changes
c) Device struct changes

I would have recommend splitting the Batch normalization changes from this PR.

py/trtorch/csrc/tensorrt_classes.h

andi4191 · 2021-01-22T19:17:02Z

py/trtorch/csrc/tensorrt_classes.h

@@ -59,7 +69,7 @@ struct Device : torch::CustomClassHolder {
        allow_gpu_fallback(false) // allow_gpu_fallback
  {}

-  ADD_FIELD_GET_SET(device_type, DeviceType);
+  ADD_ENUM_GET_SET(device_type, DeviceType, 1);


What is "1" here?
Can we use enumeration?

This macro creates a getter and setter pair that returns integers so that the function is torchbind compatible. the 1 is the max allowable value so that you dont get invalid ones.

andi4191 · 2021-01-22T19:17:41Z

py/trtorch/csrc/tensorrt_classes.h

  ADD_FIELD_GET_SET(refit, bool);
  ADD_FIELD_GET_SET(debug, bool);
  ADD_FIELD_GET_SET(strict_types, bool);
-  ADD_ENUM_GET_SET(capability, EngineCapability, 3);
+  ADD_ENUM_GET_SET(capability, EngineCapability, 2);


How about using enumerations instead of hardcoded numbers?

We could do like a static cast to int of the enums, sounds like a good idea

narendasan · 2021-01-22T19:46:49Z

The batchnorm and device checks are both in service of fixing to_backend so I think they are related. Like you couldnt use the to_backend api without both of these changes together so I think its alright that they are in the same PR. The version bump is minor, so i am not sure if its a big enough deal to review separately.

narendasan · 2021-01-22T19:48:54Z

tests/py/test_to_backend_api.py

@@ -19,8 +19,11 @@ def setUp(self):
                    "refit": False,
                    "debug": False,
                    "strict_types": False,
-                    "allow_gpu_fallback": True,
-                    "device_type": "gpu",
+                    "device": {


@andi4191 This struct looks correct to you right?

I think you are missing dla_core.

Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

andi4191

LGTM

narendasan added 5 commits January 21, 2021 19:33

feat(//py): [to_backend] adding device specification support for

6eeba1c

to_backend Also fixes nested dictionary bug reported in #286 Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>

feat(//core/lowering): Adding a new pass to handle new dim checks for

3d14cda

batchnorm Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>

refactor!: Update bazel and trt versions

0618b6b

BREAKING CHANGE: Version of bazel has been bumped to 4.0.0 Version of TensorRT has been bumped to 7.2.2.3 Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>

refactor: Lint code

0815680

Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>

github-actions bot approved these changes Jan 22, 2021

View reviewed changes

andi4191 suggested changes Jan 22, 2021

View reviewed changes

narendasan commented Jan 22, 2021

View reviewed changes

narendasan added 2 commits January 22, 2021 15:39

refactor: Addressing PR comments

5031324

Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>

fix(//py): Fix bounds for enum macros

6b942e5

Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>

github-actions bot approved these changes Jan 22, 2021

View reviewed changes

fix(//core/lowering): fix debug message for bn dim check removal pass

86bb5b7

Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>

github-actions bot approved these changes Jan 23, 2021

View reviewed changes

narendasan requested a review from andi4191 January 23, 2021 01:12

github-actions bot approved these changes Jan 23, 2021

View reviewed changes

andi4191 approved these changes Jan 25, 2021

View reviewed changes

narendasan merged commit 20022d4 into master Jan 25, 2021

narendasan deleted the to_backend_device branch January 25, 2021 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the new device API, fixing the a nested dict issue in the existing compile phase, adding new lowering pass for bn #288

Adding the new device API, fixing the a nested dict issue in the existing compile phase, adding new lowering pass for bn #288

narendasan commented Jan 22, 2021

github-actions bot left a comment

github-actions bot left a comment

andi4191 left a comment

andi4191 Jan 22, 2021

narendasan Jan 22, 2021

andi4191 Jan 22, 2021

narendasan Jan 22, 2021

narendasan commented Jan 22, 2021

narendasan Jan 22, 2021

andi4191 Jan 22, 2021 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

andi4191 left a comment

Adding the new device API, fixing the a nested dict issue in the existing compile phase, adding new lowering pass for bn #288

Adding the new device API, fixing the a nested dict issue in the existing compile phase, adding new lowering pass for bn #288

Conversation

narendasan commented Jan 22, 2021

Description

Type of change

Checklist:

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

andi4191 left a comment

Choose a reason for hiding this comment

andi4191 Jan 22, 2021

Choose a reason for hiding this comment

narendasan Jan 22, 2021

Choose a reason for hiding this comment

andi4191 Jan 22, 2021

Choose a reason for hiding this comment

narendasan Jan 22, 2021

Choose a reason for hiding this comment

narendasan commented Jan 22, 2021

narendasan Jan 22, 2021

Choose a reason for hiding this comment

andi4191 Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

andi4191 left a comment

Choose a reason for hiding this comment

andi4191 Jan 22, 2021 •

edited

Loading