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.

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

Remove MakeContiguous before CPU inputs of GPU ops. #5590

Merged
merged 5 commits into from
Aug 2, 2024

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Aug 1, 2024

Category:

Refactoring (Redesign of existing code that doesn't affect functionality)

Description:

  • Remove MakeContiguous before CPU inputs of GPU ops.
  • Remove redundant (and dangerous) checks in workspace_policy.

Before the unification of TensorList it was necessary to insert a MakeContiguous operator between CPU and other backends because TensorVector needed to be converted to TensorList. It's no longer the case and the operation is superfluous.
The checks used a no-op function in workspace_policy now kicked in and created an invalid workspace - they are no longer necessary and were removed.

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

test_pipeline.py - all of it

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Checklist

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • RST
    • Jupyter
    • Other
  • N/A

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: DALI-4030

mzient added 2 commits August 1, 2024 22:22
…and errneous checks in workspace_policy.

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Comment on lines -252 to -255
TEST_F(PipelineTestOnce, TestTriggerToContiguous) {
RunTestTrigger("cpu");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test checked for the presence of the feature we've just removed.

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
@mzient mzient force-pushed the simplify_input_setup branch from 8e3385f to 1524201 Compare August 2, 2024 08:55
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [17143006]: BUILD STARTED

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [17143849]: BUILD STARTED

Comment on lines -388 to -389
if (device == "gpu" && separated_execution_)
SetupCPUInput(it, input_idx, &spec);
Copy link
Contributor Author

@mzient mzient Aug 2, 2024

Choose a reason for hiding this comment

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

I introduced a delay in Executor RunGPU (in different places) and tested it with the following code:

@params((2, 5), (5, 2))
def test_separated_queues(cpu_queue_depth, gpu_queue_depth):
    data = [np.array((10 * (i + 1)), dtype=np.float32) for i in range(10)]
    @pipeline_def(batch_size=1, num_threads=1, device_id=0)
    def pipe():
        inp = fn.external_source(data, batch=False, cycle=True) + 0
        img = types.Constant(np.zeros(shape=(1,1,3), dtype=np.uint8)).gpu()
        return fn.resize(img, resize_x=inp, resize_y=1)  # pass argument input to resize_x

    p = pipe(prefetch_queue_depth={"cpu_size":cpu_queue_depth, "gpu_size":gpu_queue_depth})
    p.build()
    for i in range(10):
        o, = p.run()
        assert o[0].shape()[1] == data[i]

The results were correct, so this seems not necessary now (I wonder if it ever was).

I'm not adding this test to our regular tests, because it's of little value without the delays (which we certainly don't want to add).

@klecki klecki assigned klecki and unassigned szalpal Aug 2, 2024
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [17143849]: BUILD PASSED

@mzient mzient merged commit 0e2a2fe into NVIDIA:main Aug 2, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants