Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable end to end non-DPS testing #289

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Enable end to end non-DPS testing #289

wants to merge 2 commits into from

Conversation

jhalakpatel
Copy link
Collaborator

@jhalakpatel jhalakpatel commented Oct 18, 2024 •
edited
Loading

Implement python binding changes to allow execute function return
multiple returns. Update tests to use non-DPS style calling convention.

Also, enable end to end lowering by enabling conversion of closed alloc group op to tensorrt dialect.

Miscellaneous fixes:

  1. Add missing handling of CallAllocOp in EliminateShapeOps pass.
  2. Skip non ranked tensor type function arguments while collecting host tensor arguments.
  3. Temporarily add a pass to remove clone operation in MemRefToExecutor dialect conversion.
  4. Relax memref creation for empty shape tensors.
  5. Fix memref life returned from Lua function results. This required session allocator to track returned memref.
  6. Return error status instead of silently erroring out.

@jhalakpatel jhalakpatel force-pushed the jhalakp-python-exec-non-dps branch from 406ef76 to ad0cdfb Compare November 4, 2024 21:06
@jhalakpatel jhalakpatel force-pushed the jhalakp-python-exec-non-dps branch from ad0cdfb to fcb0964 Compare November 4, 2024 21:47
@jhalakpatel jhalakpatel changed the title [Python/Bindings] Allow execute_function API to return values Enable end to end non-DPS testing Nov 4, 2024
@jhalakpatel jhalakpatel force-pushed the jhalakp-python-exec-non-dps branch 2 times, most recently from 9fb24dd to 43d4c93 Compare November 5, 2024 01:05
@jhalakpatel jhalakpatel force-pushed the jhalakp-python-exec-non-dps branch 4 times, most recently from 7254cbe to ac28e6c Compare November 6, 2024 06:12
@jhalakpatel jhalakpatel force-pushed the jhalakp-python-exec-non-dps branch 4 times, most recently from e75953d to 070359d Compare November 6, 2024 22:04
@jhalakpatel jhalakpatel force-pushed the jhalakp-python-exec-non-dps branch from 070359d to d839011 Compare November 7, 2024 03:34
christopherbate
christopherbate reviewed Nov 7, 2024
View reviewed changes
mlir-tensorrt/executor/include/mlir-executor-c/Runtime/Runtime.h Outdated
@@ -53,7 +53,7 @@ extern "C" {
/// caller must be sure to delete errors via mtrtStatusDestroy.
//===----------------------------------------------------------------------===//

typedef struct MTRT_RuntimeClient MTRT_Runtimeclient;
struct MTRT_RuntimeClient; // Forward declaration
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a C header, not C++.

Without the typedef, we would fail to compile when including this header in a C library (e.g. we would need to rename all the types below to struct MTRT_RuntimeClient instead of just MTRT_RuntimeClient. The typedef is a requirement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any idea why we use MTRT_Runtimeclient instead of MTRT_RuntimeClient? Is it just a typo?

christopherbate
christopherbate reviewed Nov 7, 2024
View reviewed changes
mlir-tensorrt/executor/lib/Runtime/Backend/Lua/LuaRuntime.cpp Outdated

results.push_back(std::move(*memref));
(*client)->getAllocTracker().incrementExternalCount((*memRef)->getMemory());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this violates an invariant that we should be enforcing -- external reference count should be 0 for any "externally managed" pointer. Reference counts are only required for the object that owns the resource (which in this case is the sesssion).

I've been meaning to erase the distinction between client/session trackers and just have RuntimeSession use the Client's tracker. But until we do that, what we need here actually is to "release" the pointer from the session ownership and have the client assume ownership.

@christopherbate
Copy link
Collaborator

Overall, looks good except for minor comments. Great to see it working end-to-end, and of course much kudos to you for getting this working!

@jhalakpatel jhalakpatel force-pushed the jhalakp-python-exec-non-dps branch 5 times, most recently from 2ee6ed3 to eed9be4 Compare November 9, 2024 00:28
@jhalakpatel
Enable end to end non-DPS testing
b80b459 
Implement python binding changes to allow execute function return
multiple returns. Update tests to use non-DPS style calling convention.

Also, enable end to end lowering by enabling conversion of closed alloc
group op to tensorrt dialect.

Miscellaneous fixes:
1. Add missing handling of `CallAllocOp` in EliminateShapeOps pass.
2. Skip non ranked tensor type function arguments while collecting host
   tensor arguments.
3. Temporarily add a pass to remove clone operation in MemRefToExecutor
   dialect conversion.
4. Relax memref creation for empty shape tensors.
5. Fix memref life returned from Lua function results. This required
   session allocator to track returned memref.

Also, address
Fix incorrect indexing into output memref results
Return error status instead of silently erroring out during TensorRT weight conversion
Address review comments
@jhalakpatel jhalakpatel force-pushed the jhalakp-python-exec-non-dps branch from eed9be4 to 6e647ca Compare November 9, 2024 19:20
jhalakpatel
jhalakpatel commented Nov 9, 2024
View reviewed changes
mlir-tensorrt/executor/lib/Runtime/Backend/Lua/LuaRuntime.cpp Outdated
@@ -650,9 +650,6 @@ parseResults(const sol::protected_function_result &pfr,
if (!memref.isOk())
return memref.getStatus();

// Increment external reference count since we are returning a memref
allocator.incrementExternalCount(info.ptr);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not need this. Reference count should only be incremented if a explicitly create a DL pack tensor on a memref.
Here memref is internally owned and tracked.

@jhalakpatel jhalakpatel force-pushed the jhalakp-python-exec-non-dps branch from 7b25b15 to 9334481 Compare November 9, 2024 22:19
@jhalakpatel jhalakpatel force-pushed the jhalakp-python-exec-non-dps branch from 9334481 to 8710d9a Compare November 9, 2024 22:19
@jhalakpatel
Copy link
Collaborator Author

@christopherbate were you able to merge changes from this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christopherbate christopherbate christopherbate left review comments

@shelkesagar29 shelkesagar29 Awaiting requested review from shelkesagar29 shelkesagar29 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jhalakpatel @christopherbate