Skip to content

fix: add missing NVTE_BHSD to qkv format to_string - #3362

Open
andrewwhitecdw wants to merge 2 commits into
NVIDIA:mainfrom
andrewwhitecdw:bugfix/fused-attn-add-missing-nvte-bhsd-to-qkv-format-to
Open

fix: add missing NVTE_BHSD to qkv format to_string#3362
andrewwhitecdw wants to merge 2 commits into
NVIDIA:mainfrom
andrewwhitecdw:bugfix/fused-attn-add-missing-nvte-bhsd-to-qkv-format-to

Conversation

@andrewwhitecdw

Copy link
Copy Markdown
Contributor

This PR addresses the following issue in transformer_engine/common/fused_attn/fused_attn.cpp: add missing NVTE_BHSD to qkv format to_string.

Changes

  • transformer_engine/common/fused_attn/fused_attn.cpp: add missing NVTE_BHSD to qkv format to_string.

Details

--- a/transformer_engine/common/fused_attn/fused_attn.cpp
+++ b/transformer_engine/common/fused_attn/fused_attn.cpp
@@ -1,6 +1,8 @@
-    case NVTE_BSHD:
-      return "NVTE_BSHD";
-    case NVTE_THD:
-      return "NVTE_THD";
-    case NVTE_BSHD_2SBHD:
-      return "NVTE_BSHD_2SBHD";
+    case NVTE_BSHD:
+      return "NVTE_BSHD";
+    case NVTE_BHSD:
+      return "NVTE_BHSD";
+    case NVTE_THD:
+      return "NVTE_THD";
+    case NVTE_BSHD_2SBHD:
+      return "NVTE_BSHD_2SBHD";

Tests

  • tests/test_qkv_format_to_string.cpp
diff --git a/tests/test_qkv_format_to_string.cpp b/tests/test_qkv_format_to_string.cpp
new file mode 100644
--- /dev/null
+++ b/tests/test_qkv_format_to_string.cpp
@@ -0,0 +1,15 @@
+/*************************************************************************
+ * Copyright (c) 2022-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+ *
+ * See LICENSE for license information.
+ ************************************************************************/
+
+#include <gtest/gtest.h>
+#include <string>
+
+#include "transformer_engine/fused_attn.h"
+
+TEST(QKVFormatToString, BHSD) {
+  EXPECT_EQ(transformer_engine::to_string(NVTE_QKV_Format::NVTE_BHSD), "NVTE_BHSD");
+}
+
+TEST(QKVFormatToString, LayoutBHSD_BHSD_BHSD) {
+  EXPECT_EQ(nvte_get_qkv_format(NVTE_QKV_Layout::NVTE_BHSD_BHSD_BHSD),
+            NVTE_QKV_Format::NVTE_BHSD);
+}

Squashed to single commit for review.

Original PR: #16
@github-actions github-actions Bot added the community-contribution PRs from external contributor outside the core maintainers, representing community-driven work. label Aug 13, 2026
@greptile-apps

greptile-apps Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR adds the missing NVTE_BHSD string representation and a focused GoogleTest assertion.

  • Maps NVTE_BHSD to "NVTE_BHSD" in the fused-attention format formatter.
  • Adds regression coverage intended to validate the mapping.

Confidence Score: 4/5

The implementation is not ready to merge because the regression test remains outside every configured C++ test target and therefore never runs.

The enum mapping itself is straightforward, but the previously reported coverage failure remains: CI builds the explicitly registered tests/cpp targets, while the new root-level source is not included in any target.

Files Needing Attention: tests/test_qkv_format_to_string.cpp; tests/cpp/util/CMakeLists.txt

Important Files Changed

Filename Overview
transformer_engine/common/fused_attn/fused_attn.cpp Adds the missing enum-to-string case for the BHSD QKV format.
tests/test_qkv_format_to_string.cpp Adds a focused GoogleTest assertion for the new mapping.

Reviews (2): Last reviewed commit: "[pre-commit.ci] auto fixes from pre-comm..." | Re-trigger Greptile

Comment on lines +12 to +14
TEST(QKVFormatToString, BHSD) {
EXPECT_EQ(transformer_engine::to_string(NVTE_QKV_Format::NVTE_BHSD), "NVTE_BHSD");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Regression test is not built

This file is outside the tests/cpp CMake project, whose test targets use explicit source lists, so CI never compiles or executes this assertion and the new mapping receives no regression coverage.

Knowledge Base Used: Tests and QA

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!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This test can be removed - the addition of BHSD in the enum is a pretty obvious change and it won't make any tests fail. Also, the test probably can't compile (and it should be in the tests/cpp folder as well if we wanted to add this CPP test). Thanks!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed — the change itself is straightforward and self-evident. Removing the test file is the right call, especially since it wouldn't compile as-is and belongs in tests/cpp if it were to be kept. Thanks for the clarification!

@cyanguwa

cyanguwa commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Thanks @andrewwhitecdw for submitting PRs #3361 to #3379. I've reviewed them and I think I should make a few suggestions regarding the PR styling and CI testing.

  • Some of the PRs are one-liners, or fairly small, and they touch the same files as others. So could you please consolidate the relevant ones to one PR, and this will save rebase churns and some CI runs.
  • Not all of these PRs require adding a test. For example, the [()] -> [] change is probably safe with just some local testing, instead of bloating the CI infra.
  • Some PRs have code diffs in their descriptions. This is unnecessary because we have the "Files changed" tab, and can be misleading if they don't match what's really changed in the code.

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution PRs from external contributor outside the core maintainers, representing community-driven work.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants