Fix zero-extent GPU image launches - #6505
arcusbuilds wants to merge 2 commits into
Conversation
Skip zero-pixel samples in the shared color conversion and Flip launchers before computing launch dimensions. Add a mixed-batch regression test. Fixes NVIDIA#6504 Signed-off-by: Srijan Keshri <212402043+arcusbuilds@users.noreply.github.com>
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| TEST(ColorSpaceConversionTest, ZeroExtentSampleGPU) { | ||
| TensorList<CPUBackend> input; | ||
| input.Resize(TensorListShape<3>({{0, 2, 3}, {1, 1, 3}}), DALI_UINT8); | ||
| input.SetLayout("HWC"); | ||
| auto *pixel = input.mutable_tensor<uint8_t>(1); | ||
| pixel[0] = 1; | ||
| pixel[1] = 2; | ||
| pixel[2] = 3; | ||
|
|
||
| Pipeline pipe(2, 1, 0); | ||
| pipe.AddExternalInput("input"); | ||
| pipe.AddOperator(OpSpec("Flip") | ||
| .AddArg("device", "gpu") | ||
| .AddInput("input", StorageDevice::GPU) | ||
| .AddOutput("flipped", StorageDevice::GPU)); | ||
| pipe.AddOperator(OpSpec("ColorSpaceConversion") | ||
| .AddArg("device", "gpu") | ||
| .AddArg("image_type", DALI_RGB) | ||
| .AddArg("output_type", DALI_BGR) | ||
| .AddInput("flipped", StorageDevice::GPU) | ||
| .AddOutput("output", StorageDevice::GPU)); | ||
| pipe.Build(std::vector<std::pair<std::string, std::string>>{{"output", "gpu"}}); | ||
| pipe.SetExternalInput("input", input); | ||
|
|
||
| Workspace ws; | ||
| pipe.Run(); | ||
| pipe.Outputs(&ws); | ||
| TensorList<CPUBackend> output; | ||
| output.Copy(ws.Output<GPUBackend>(0)); | ||
| EXPECT_EQ(output.num_samples(), 2); | ||
| EXPECT_EQ(output.tensor_shape(0), input.tensor_shape(0)); | ||
| EXPECT_EQ(output.tensor_shape(1), input.tensor_shape(1)); | ||
| const auto *converted = output.tensor<uint8_t>(1); | ||
| EXPECT_EQ(converted[0], 3); | ||
| EXPECT_EQ(converted[1], 2); | ||
| EXPECT_EQ(converted[2], 1); | ||
| } |
There was a problem hiding this comment.
This new operator test only covers a mixed GPU batch. The repository's test requirements also require new operator tests to cover empty input, batch_size=1, boundary values, CPU and GPU variants, and invalid-input rejection. Add the missing cases in the appropriate test suites before merging.
Rule Used: New operator tests must cover: empty input, single sample (batch_size=1), boundary values, CPU and GPU variants, and invalid-input rejection (via assert_raises(..., glob=...) with the expected message pattern). (source)
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!
There was a problem hiding this comment.
This is a targeted C++ regression for zero-extent GPU samples: the new tests cover a mixed batch and a single zero-extent sample with an unsupported conversion. Existing tests cover ordinary CPU/GPU conversions. The new GPU tests have not run locally;
NVIDIA CI is awaiting workflow vetting.
There was a problem hiding this comment.
That context is reasonable. The mixed two-sample GPU test exercises the zero-extent early return together with a nonempty sample and verifies the converted result, while ZeroExtentUnsupportedConversionGPU confirms that the guard does not bypass conversion validation. Existing parameterized tests provide ordinary CPU/GPU conversion coverage, so requiring separate boundary-value, batch-size-one, and invalid-input matrices here was overly broad for this targeted regression. I’m withdrawing the “required test cases missing” request; the added tests are sufficient for this fix.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
There was a problem hiding this comment.
That’s fair. The existing typed GenericConversionTest suites already exercise the supported CPU/GPU conversion paths, while these additions specifically cover the new zero-extent GPU behavior: a mixed batch through both Flip and conversion, plus preservation of the unsupported-conversion error for an empty sample. The broader empty-input, boundary-value, and batch_size=1 matrix I requested is not necessary to validate this targeted launcher regression, so I’m withdrawing that comment.
This comment has been minimized.
This comment has been minimized.
Keep unsupported conversion pairs rejected when the pixel count is zero, and cover the case in the existing GPU operator test. Signed-off-by: Srijan Keshri <212402043+arcusbuilds@users.noreply.github.com>
| conversion != kYCbCr_TO_BGR && conversion != kYCbCr_TO_RGB && | ||
| conversion != kYCbCr_TO_GRAY && conversion != kGRAY_TO_RGB && | ||
| conversion != kGRAY_TO_BGR && conversion != kGRAY_TO_YCbCr) { | ||
| DALI_FAIL(make_string("conversion not supported ", in_type, " to ", out_type)); |
There was a problem hiding this comment.
The new zero-pixel validation uses DALI_FAIL for a user-facing input error and reports only conversion not supported <input> to <output>. Repository requirements say new validation must use a specific exception or typed DALI error, and the message must identify the operator or parameters, show the supported versus actual values, and end with a complete sentence. This requirement must be satisfied before merging.
Rule Used: assert is for internal invariants (compiled out in release). User-facing input validation must throw — prefer specific C++ exceptions (std::invalid_argument, std::out_of_range, std::runtime_error) or DALI errors that build on them (e.g., `dal... (source)
Knowledge Base Used: Core runtime and kernel library
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!
There was a problem hiding this comment.
The zero-pixel path intentionally uses the same DALI_FAIL and message as the existing unsupported-conversion fallback in this launcher (line 217). That keeps the error behavior consistent for empty and non-empty inputs. Changing the exception type and message in both paths would be a separate change.
There was a problem hiding this comment.
That’s a valid clarification. The zero-pixel guard must still reject unsupported conversions before returning, since otherwise the early return would bypass the existing fallback at line 217. Reusing the same DALI_FAIL and message keeps behavior consistent between empty and non-empty inputs, so the validation concern does not apply here. I’m retracting this comment; no change is needed for this path.
3 participants
Category
Bug fix
Description
Return from the shared GPU Flip and color conversion launchers when a sample has zero pixels, before computing launch dimensions. This prevents host-side division by zero for zero-extent samples in a nonempty batch. Fixes #6504.
Additional information
The guards are in the launchers, so all callers use them. A mixed-batch GTest sends an empty sample and a normal RGB pixel through Flip and color conversion, then checks the BGR output. A second test checks that an unsupported conversion still fails for an empty sample.
Tests
ColorSpaceConversionTest.ZeroExtentSampleGPU,ColorSpaceConversionTest.ZeroExtentUnsupportedConversionGPUChecklist
REQ IDs: N/A
JIRA TASK: N/A