ci: Guard dynamic CUDART isolation - #1360
Conversation
| build-type: | ||
| - release | ||
| - debug | ||
| cudart: | ||
| - default |
There was a problem hiding this comment.
This should be a build types because that what it is. We just need to test in CI accordingly.
There was a problem hiding this comment.
Didn't want to multiply the test dimension and simply introduce a single test row to catch any cuda runtime api usage. Can you explain a little more about it being in built-type? To be consistent with the convention?
There was a problem hiding this comment.
I think the correct approach is establishing the build type with the possible build options (look at the autotools configure options we have set) and then executing only what we need. If you are worried about build times blowing up I think we can cut OS's for things like LTTNG/NVTX. Those are mainly for internal debugging use so we cant limit those. This way we can still get our coverage on OS's but be intentional about it.
| runs-on: ubuntu-latest | ||
| container: ghcr.io/${{ github.repository }}/aws-ofi-nccl-al2023:${{ matrix.sdk }}-efa${{ matrix.efainstaller }} | ||
| name: al2023/${{ matrix.sdk }}/efa@${{ matrix.efainstaller }}/${{ matrix.build-type }} | ||
| name: al2023/${{ matrix.sdk }}/efa@${{ matrix.efainstaller }}/${{ matrix.build-type }}${{ matrix.cudart == 'dynamic' && '/cudart-dynamic' || '' }} |
There was a problem hiding this comment.
don't be silent about the cudart config in the non-dynamic case.
There was a problem hiding this comment.
I have named the non-dynamic as well now it shows like:
al2023/cuda/efa@latest/release/cudart-static
and
al2023/cuda/efa@latest/release/cudart-dynamic
However, I didn't put any names in the other stages such as ubuntu and/or neuron. Please let me know if its ok?
There was a problem hiding this comment.
Well, I don't understand why we're only running this test on one OS....
There was a problem hiding this comment.
I didn't want to multiply the test matrix. However, I have now added the check in all Ubuntu OSes as well but for only one stage that is gcc/release. Please let me know if we really need to multiply it across the test matrix. There will be around 150 more checks.
72757c6 to
03cbb96
Compare
03cbb96 to
e10b026
Compare
The default CUDA build links CUDART statically, which allows accidental CUDA Runtime calls in the core plugin to pass the existing CI matrix. Binary packages use --enable-cudart-dynamic and require the plugin DSOs to avoid a link-time dependency on the CUDA Runtime version used to build them. Exercise both CUDART modes across every CUDA combination in the Amazon Linux and Ubuntu matrices while excluding dynamic CUDART from Neuron builds. Label each CUDA job with its CUDART mode and check the resulting core DSOs for a direct libcudart dependency so this class of regression fails in plugin CI instead of downstream packaging. Signed-off-by: Bibrak Qamar Chandio <bibracha@amazon.com>
e10b026 to
ae850c8
Compare
4 participants
Description of changes:
The default CUDA build links CUDART statically, which allows accidental CUDA Runtime calls in the core plugin to pass the existing CI matrix. Binary packages use --enable-cudart-dynamic and require the plugin DSOs to avoid a link-time dependency on the CUDA Runtime version used to build them.
Add one dynamic-CUDART variant to the Amazon Linux matrix while keeping all existing default builds. Check the resulting core DSOs for a direct libcudart dependency or unresolved CUDA Runtime symbols so this class of regression fails in plugin CI instead of downstream packaging.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.