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

Fix stream usage in C API #3713

Merged
merged 4 commits into from
Mar 2, 2022
Merged

Fix stream usage in C API #3713

merged 4 commits into from
Mar 2, 2022

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Mar 1, 2022

Category:

Bug fix (non-breaking change which fixes an issue)

Description:

  • Stream value of 0 should no longer be converted to an AccessOrder in C API.
  • Made C API tests that don't use CUDA usable without a CUDA driver (most CPU Backend tests - exclusing those that use copy kernel or both backends).
  • Adjust cpplint to support C++17 structured bindings with space in auto [a, b]

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Checklist

Tests

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

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: N/A

… CPU backen usable without a GPU.

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
@JanuszL JanuszL self-assigned this Mar 1, 2022
@@ -69,9 +69,27 @@ std::string GetDeviceStr(device_type_t dev) {
return dev == CPU ? "cpu" : "gpu";
}

// Allocates a buffer on the specified backend and, if necessary, another one for the CPU
Copy link
Contributor

Choose a reason for hiding this comment

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

The only test we run from this file without GPU is CpuOnlyTest - see https://github.com/NVIDIA/DALI/blob/main/qa/TL0_cpu_only/test.sh#L34.
Maybe we should improve the pattern there - or change test naming here to run more CPU only test?

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [4061845]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [4061845]: BUILD FAILED

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

CI MESSAGE: [4062458]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [4062458]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [4062458]: BUILD PASSED

@jantonguirao jantonguirao added the important-fix Fixes an important issue in the software or development environment. label Mar 2, 2022
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [4068049]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [4068293]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [4068293]: BUILD FAILED

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

CI MESSAGE: [4068460]: BUILD STARTED

@@ -31,7 +31,7 @@ test_body() {
exit 1
fi

"$FULLPATH" --gtest_filter="*CpuOnlyTest*"
"$FULLPATH" --gtest_filter="*CpuOnly*:*CApi*/0.*-*0.UseCopyKernel:*ForceNoCopyFail:*daliOutputCopySamples"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does 0 mean 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.

Yes - unfortunately the name of the TypeParam is not a part of the name, so you can't match against that.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can pls add a comment about that?

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [4068049]: BUILD PASSED

@mzient mzient merged commit dafb413 into NVIDIA:main Mar 2, 2022
JanuszL pushed a commit that referenced this pull request Mar 2, 2022
* Fix stream/order usage in C API and tests. Make most C API tests with CPU backen usable without a GPU.
* Make C API CpuOnly test only run for CPU backend.
* Use AccessOrder in copyX2Y. Add more CPU-only tests.
* Extend the range of CPU-only native tests
* Adjust cpplint for C++17 structured bindings.

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

CI MESSAGE: [4068460]: BUILD PASSED

cyyever pushed a commit to cyyever/DALI that referenced this pull request May 13, 2022
* Fix stream/order usage in C API and tests. Make most C API tests with CPU backen usable without a GPU.
* Make C API CpuOnly test only run for CPU backend.
* Use AccessOrder in copyX2Y. Add more CPU-only tests.
* Extend the range of CPU-only native tests
* Adjust cpplint for C++17 structured bindings.

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jun 7, 2022
* Fix stream/order usage in C API and tests. Make most C API tests with CPU backen usable without a GPU.
* Make C API CpuOnly test only run for CPU backend.
* Use AccessOrder in copyX2Y. Add more CPU-only tests.
* Extend the range of CPU-only native tests
* Adjust cpplint for C++17 structured bindings.

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
important-fix Fixes an important issue in the software or development environment.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants