Fix Cat and Stack reading the layout only from the first input - #6479
Fix Cat and Stack reading the layout only from the first input#6479rootkiller6788 wants to merge 4 commits into
Conversation
The loop in TensorJoin::GetInputLayout was meant to go over all inputs, but it fetched input 0 every iteration. As a result a layout carried only by a later input was dropped, and two non-empty layouts that disagreed were never flagged.
Covers a layout that lives only on the second input for both Cat and Stack, and conflicting non-empty layouts being rejected with an error.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@rootkiller6788 thank you for your contribution. Can you please check Greptile review comments? |
The first version used the old Pipeline context manager and set_outputs, which the repo asks new tests to avoid. Rewrote them around a small helper that builds the pipeline, so the upcoming cases are easy to add. Signed-off-by: rootkiller6788 <c8688rickowens@outlook.com>
Single-sample batches, a 1x1 and an empty sample, plus a negative axis for Cat. The layout conflict is now checked on both devices, and for Stack as well - that one only looks at the input layouts when axis_name is given, so it needs the name set to be covered at all. Signed-off-by: rootkiller6788 <c8688rickowens@outlook.com>
| outputs.append(op(*operands, **kwargs)) | ||
| return tuple(outputs) | ||
|
|
||
| return join_pipeline |
There was a problem hiding this comment.
_join_pipeline returns the function produced by @pipeline_def, but callers treat it as a constructed pipeline and call pipe.run(). Because the decorated function must first be invoked, these tests raise AttributeError: 'function' object has no attribute 'run' before exercising Cat or Stack.
| return join_pipeline | |
| return join_pipeline() |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def join_pipeline(): | ||
| inputs = [ | ||
| _layout_input(shape, value, batch_size, layout) | ||
| for value, layout in zip(values, layouts) |
There was a problem hiding this comment.
This loop assumes values and layouts have equal lengths but uses plain zip, which can silently omit inputs if the lengths diverge. The repository directive requires strict=True when paired sequences are expected to have equal cardinality, and that requirement must be satisfied before merging.
| for value, layout in zip(values, layouts) | |
| for value, layout in zip(values, layouts, strict=True) |
Rule Used: Use zip(a, b, strict=True) (Python 3.10+) when i... (source)
|
Thanks for the review — both points are addressed. The new tests now build their pipelines with |
| outputs.append(op(*operands, **kwargs)) | ||
| return tuple(outputs) | ||
|
|
||
| return join_pipeline |
There was a problem hiding this comment.
[Critical] _join_pipeline returns join_pipeline — the function object produced by @pipeline_def — not a constructed Pipeline. Per pipeline_def's own docstring, the decorated function must be called to get a Pipeline (pipe = my_pipe(...)). Every caller here does pipe = _join_pipeline(...) then pipe.run(), so all four new tests (test_cat_layout_from_any_input, test_stack_layout_from_any_input, test_cat_layout_mismatch, test_stack_layout_mismatch) fail immediately with AttributeError: 'function' object has no attribute 'run' before touching Cat or Stack. Confirmed independently of Greptile's suggestion below — same fix: return join_pipeline().
4 participants
Cat and Stack only looked at the first input when deciding the output layout. GetInputLayout loops over every input but grabbed input 0 in each iteration, which meant:
The change makes the loop read input i instead of input 0. I added tests covering a layout carried only by the second input (for both Cat and Stack) and the conflicting-layout error path.