[torchlib] Add missing ops (im2col) #1757
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1757 +/- ##
==========================================
+ Coverage 75.04% 75.08% +0.03%
==========================================
Files 245 245
Lines 26472 26516 +44
Branches 4829 4834 +5
==========================================
+ Hits 19867 19910 +43
- Misses 5674 5675 +1
Partials 931 931 ☔ View full report in Codecov by Sentry. |
Test Results 24 files ± 0 24 suites ±0 3h 38m 1s ⏱️ + 23m 7s For more details on these failures, see this check. Results for commit 601f5d3. ± Comparison against base commit 14f88d3. This pull request removes 2278 and adds 98 tests. Note that renamed tests count towards both.This pull request removes 475 skipped tests and adds 45 skipped tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
| output = op.Gather(padded_input, blocks_row_indices, axis=2) | ||
| output = op.Gather(output, blocks_col_indices, axis=4) |
There was a problem hiding this comment.
Possible to use slice, which is faster?
There was a problem hiding this comment.
I think we can use Slice, however the indices would need to be transformed to starts, ends format adding extra Reshape and Split nodes
There was a problem hiding this comment.
I see. Then this lgtm. Thanks for explaining!
There was a problem hiding this comment.
But the extra-operations can be done at export-time, is that correct? That is, they depend only on export-time values (torch parameters == onnx attributes), and not on run-time values. If so, there is no need to encode them using onnx ops, as it can be done in Python? In other words, using Slice should be doable in the trace-mode without any extra cost?
There was a problem hiding this comment.
I think this is worth thinking through.
- Slice is an operation that has a very regular access pattern that is easier to optimize and parallelize. But Gather is very irregular and random, harder to optimize and parallelize.
- The cost of operations on a large input tensor dominate overall cost, not cost of constant-time operations like Reshape.
- If we want to extract a million elements, creating the indices of these million elements seems potentially expensive, when it can be described using a slice-pattern with a few elements.
There was a problem hiding this comment.
But if the entire model consists of a single op-function call? I wasn't necessarily looking for something visual. Just knowing impl1 takes X time and impl2 takes Y time would be fine. The starting point would be a test-case for an op like im2col, we run its onnxscript impl exported to ORT as a model.
There was a problem hiding this comment.
I wonder how correlated a tiny bench is with the e2e performance? Hopefully closely?
There was a problem hiding this comment.
Good question. We will need to avoid overheads (like copying tensors, eg. due to conversion, etc.). And not count session creation (which should be easy). May be even warm up. Should be doable.
There was a problem hiding this comment.
Hi @shubhambhokare1 : I see this has been merged. I am concerned that the strategy used here might not be good, for reasons discussed above. Any thoughts about that? Thanks!
There was a problem hiding this comment.
Hi @gramalingam,
Agreed with the point about the case with a large number of elements, creating these indices and using gather might be inefficient. I think I must have missed this comment thread pre-merge. Slice might be a better option.
Will add a PR on top of this to remedy this, replacing the gathers ops with slice, I guess models using im2col should be unblocked for now.
In regards to the second point, might be a good idea to create a single-op based evaluator for kernel performance. Will experiment and add that as part of the new PR.
shubhambhokare1
force-pushed
the
sbhokare/torchlib-op-3
branch
from
219868e to
ffe8a7f
Compare
shubhambhokare1
force-pushed
the
sbhokare/torchlib-op-3
branch
from
ffe8a7f to
41200fc
Compare
Co-authored-by: Justin Chu <[email protected]>
justinchuby
added
the
topic: torch_lib
No description provided.