feat(serde)!: Refactor CudaDevice struct, implement ABI versioning, serde cleanup #520
Conversation
serde cleanup BREAKING CHANGE: This commit cleans up the WIP CudaDevice class, simplifying implementation and formalizing the seralized format for CUDA devices. It also implements ABI Versioning. The first entry in the serialized format of a TRTEngine now records the ABI that the engine was compiled with, defining expected compatibility with the TRTorch runtime. If the ABI version does not match, the runtime will error out asking to recompile the program. ABI version is a monotonically increasing integer and should be incremented everytime the serialization format changes in some way. This commit cleans up the CudaDevice class, implementing a number of constructors to replace the various utility functions that populate the struct. Descriptive utility functions remain but solely call the relevant constructor. Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
github-actions
bot
added
component: api [Python]
core/runtime/register_trt_op.cpp
Outdated
| << ". Switching context"); | ||
| return true; | ||
| } | ||
| // REVIEW: This shouldnt be a reason to switch GPU |
There was a problem hiding this comment.
I wasnt sure why we have this check
There was a problem hiding this comment.
If the device_id configured are not the same, then the memory allocations need to move between the devices. Hence, confirmation for switching the context is needed.
There was a problem hiding this comment.
We should probably make the warning more explanatory then
| // Set Device Type | ||
| this->device_type = device_type; | ||
| } | ||
|
|
There was a problem hiding this comment.
Switched to a delimted string vs. a byte indexed string, should be easier to parse
There was a problem hiding this comment.
It still requires parsing based on the positional encoding of the parameters during deserialization.
core/runtime/register_trt_op.cpp
Outdated
| << ". Switching context"); | ||
| return true; | ||
| } | ||
| // REVIEW: This shouldnt be a reason to switch GPU |
There was a problem hiding this comment.
If the device_id configured are not the same, then the memory allocations need to move between the devices. Hence, confirmation for switching the context is needed.
| // Set Device Type | ||
| this->device_type = device_type; | ||
| } | ||
|
|
There was a problem hiding this comment.
It still requires parsing based on the positional encoding of the parameters during deserialization.
core/runtime/runtime.cpp
Outdated
| } | ||
|
|
||
| CudaDevice get_current_device() { | ||
| int device = 0; |
There was a problem hiding this comment.
Probably should be -1
core/runtime/register_trt_op.cpp
Outdated
| << ". Switching context"); | ||
| return true; | ||
| } | ||
| // REVIEW: This shouldnt be a reason to switch GPU |
There was a problem hiding this comment.
We should probably make the warning more explanatory then
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
There are some changes that do not conform to C++ style guidelines:
diff --git a/workspace/core/runtime/register_trt_op.cpp b/tmp/changes.txt
index 0241ca3..ce92c42 100644
--- a/workspace/core/runtime/register_trt_op.cpp
+++ b/tmp/changes.txt
@@ -35,7 +35,7 @@ bool is_switch_required(const CudaDevice& curr_device, const CudaDevice& conf_de
if (curr_device.id != conf_device.id) {
LOG_WARNING(
"Configured Device ID: " << conf_device.id << " is different that current device ID: " << curr_device.id
- << ". Moving input tensors to device: " << conf_device.id);
+ << ". Moving input tensors to device: " << conf_device.id);
return true;
}
diff --git a/workspace/core/runtime/CudaDevice.cpp b/tmp/changes.txt
index 95ed54d..496c217 100644
--- a/workspace/core/runtime/CudaDevice.cpp
+++ b/tmp/changes.txt
@@ -73,11 +73,11 @@ std::string CudaDevice::serialize() {
content[ID_IDX] = std::to_string(id);
content[SM_MAJOR_IDX] = std::to_string(major);
content[SM_MINOR_IDX] = std::to_string(minor);
- content[DEVICE_TYPE_IDX] = std::to_string((int64_t) device_type);
+ content[DEVICE_TYPE_IDX] = std::to_string((int64_t)device_type);
content[DEVICE_NAME_IDX] = device_name;
std::stringstream ss;
- for(size_t i = 0; i < content.size() - 1; i++) {
+ for (size_t i = 0; i < content.size() - 1; i++) {
ss << content[i] << DEVICE_INFO_DELIM;
}
ss << content[DEVICE_NAME_IDX];
ERROR: Some files do not conform to style guidelinesSigned-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>
narendasan
force-pushed
the
narendasan/device_serde
branch
from
771e67b to
8d3258e
Compare
Description
BREAKING CHANGE: This commit cleans up the WIP CudaDevice class,
simplifying implementation and formalizing the seralized format for CUDA
devices.
It also implements ABI Versioning. The first entry in the serialized
format of a TRTEngine now records the ABI that the engine was compiled
with, defining expected compatibility with the TRTorch runtime. If the
ABI version does not match, the runtime will error out asking to
recompile the program.
ABI version is a monotonically increasing integer and should be
incremented everytime the serialization format changes in some way.
This commit cleans up the CudaDevice class, implementing a number of
constructors to replace the various utility functions that populate the
struct. Descriptive utility functions remain but solely call the
relevant constructor.
Fixes # (issue)
Type of change
Please delete options that are not relevant and/or add your own.
Checklist: