Enable CI lint gh action on ROCm by VeeraRajasekhar · Pull Request #547 · ROCm/TransformerEngine

VeeraRajasekhar · 2026-04-17T06:05:09Z

Description

Please include a brief summary of the changes, relevant motivation and context.

Fixes # (issue)

Type of change

Documentation change (change only to the documentation, either a fix or a new content)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Infra/Build change
Code refactoring

Changes

Please list the changes introduced in this PR:

Change A
Change B

Checklist:

I have read and followed the contributing guidelines
The functionality is complete
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

transformer_engine/common/amd_detail/hip_float8.h -Host constructor: multi-statement if/else now uses braces (readability/braces). transformer_engine/common/cast/mxfp8/rocm_quantize_mxfp8.cuh -Include <cstdint>; typedef for gfx950 vector type uses int16_t instead of short (runtime/int). transformer_engine/common/ck_fused_attn/src/ck_fused_attn_utils.cpp -dladdr: avoid ill-formed function-pointer-to-void* cast via a small union (readability/casting / portable POSIX). -get_ck_log_stream: else branch restructured with nested if so else/brace pairing satisfies cpplint (readability/braces). transformer_engine/common/fused_attn_rocm/fused_attn.cpp -check_set_window_size: replace std::make_pair<int64_t,int64_t>(...) with std::pair<int64_t,int64_t>(...) (build/explicit_make_pair). -Replace alternative tokens `or` with || (readability/alt_tokens). -log_fused_attn_config: same for sliding-window condition. transformer_engine/common/gemm/rocm_gemm.cu -ObjCache / NameMapper: mark single-argument constructors explicit (runtime/explicit). -HIPBLASLT scaling_mode check: split #if/#else branches so each if has its own braced body; use static_cast<int> instead of C-style cast (readability/braces, readability/casting). -Debug logging: (int) casts -> static_cast<int> for hipDataType fields (readability/casting). -ServiceStreamKey: use std::uint64_t alias instead of unsigned long long (runtime/int). transformer_engine/common/normalization/common.cpp -getNormalizationPlan: after optional CUDNN plan, use if (!plan) { ... } for TE plans instead of } else #endif if (readability/braces across preprocessor). transformer_engine/common/normalization/layernorm/ln_api.cpp -Forward/backward: default norm_backend to Te; optional CUDNN path only under #ifndef __HIP_PLATFORM_AMD__; set is_aligned only when backend is Te, so preprocessor does not split if/else from its braces (readability/braces). transformer_engine/common/normalization/rmsnorm/rmsnorm_api.cpp -Same pattern as ln_api for forward (including HIP constexpr gamma_in_weight_dtype) and backward cudnn vs Te (readability/braces). transformer_engine/common/permutation/permutation.cu -MoE unpermute kernel: functional-style float(...) casts replaced with static_cast<float>(...) (readability/casting). transformer_engine/common/util/logging.h -NVTE_CHECK_HIPBLASLT macro: std::to_string((int)status) -> std::to_string(static_cast<int>(status)) (readability/casting). transformer_engine/pytorch/csrc/extensions/gemm.cpp -Comm overlap RS path: HIP p2p vs split_overlap_rs restructured with proper #else for non-HIP so } else #endif { does not confuse brace rules (readability/braces).

Micky774

Does this cause divergence from upstream's source code in inherited files? Also, why do we need linting as a CI action instead of e.g. a pre-commit?

Micky774 · 2026-04-17T14:16:33Z

+    // dladdr expects void*; avoid reinterpret_cast<void*>(fn) (not ISO C++).
+    union {
+      void (*fn)();
+      void *addr;
+    } sym{};
+    sym.fn = set_aiter_asm_dir;
+    dladdr(sym.addr, &info);


IMO this is unnecessary. Yes, it quiets the warning, but the warning is irrelevant for us granted our support is POSIX focused to begin with. From the dlopen man page:

/* According to the ISO C standard, casting between function pointers and 'void *', as done above, produces undefined results. POSIX.1-2001 and POSIX.1-2008 accepted this state of affairs and proposed the following workaround: *(void **) &cosine = dlsym(handle, "cos"); This (clumsy) cast conforms with the ISO C standard and will avoid any compiler warnings. The 2013 Technical Corrigendum 1 to POSIX.1-2008 improved matters by requiring that conforming implementations support casting 'void *' to a function pointer. Nevertheless, some compilers (e.g., gcc with the '-pedantic' option) may complain about the cast used in this program. */

the union trick here provides no additional safety -- it's still undefined behavior technically speaking -- and will break in the same circumstances (non-POSIX risk).

All things considered, I'd rather we keep things as-is, and if we really want to deal with the warning, we can make a small utility to use pragmas to suppress the warnings locally around the cast.

Micky774 · 2026-04-17T14:29:25Z

  }

-  ObjCache(void (*a_offload)(const Data&)): offload(a_offload) {}
+  explicit ObjCache(void (*a_offload)(const Data&)): offload(a_offload) {}


Do we really want these constructors to be explicit? Are they even used implicitly anywhere in our codebase?

they aren’t used implicitly anywhere; the only constructions are direct ObjCache<T,K>(nullptr) from ObjPool and direct init of service_stream_cache with a lambda. explicit was added only to satisfy cpplint runtime/explicit. If we prefer not to mark these callbacks as explicit, we can drop explicit and suppress that line with NOLINT for cpplint instead.

There are no APIs that take an ObjCache by value. So explicit is not required for correctness, only for style / tooling.

Micky774 · 2026-04-17T14:31:00Z

I'm assuming this won't be part of the final PR?

Yes, This is for me to keep track of the issues fixed, will remove this.

- common: trailing space, encoding, docstrings, wheel file handle naming - recipe: Format helper docstrings, whitespace - jax/util: PEP8, module docstring, subprocess check, def over lambdas - jax/setup: group build_tools.hipify imports before pybind11 - jax/quantize/helper: tejax + CUDA/ROCm helpers, no-else-return - cpp_extensions: attention/normalization lazy SdyShardingRule; base is_hip_extension(); gemm conditional cGEMM imports + stubs - pylintrc: align disables (e.g. wrong-import-position) with CI.

ipanfilo · 2026-04-22T02:18:52Z

        if (log_dir_str == "1") {
          log_stream = &std::cout;
-        }
-        else if (open_ck_fused_attn_log_file(log_file, "ck_fused_attn", log_dir_str)) {


What is a warning for if-else if? I think it is used a lot in our code

The category was readability/braces. The message was along the lines of: “If an else has a brace on one side, it should have it on both.”

So the warning showed up because cpplint’s readability/braces heuristic fired on this if / else if layout, not because else if is forbidden. Nesting as else { if (...) { ... } } makes the structure obvious to the linter and cleared the warning.

Move them to single line then but do not create nested ifs

ipanfilo · 2026-04-22T02:36:32Z

      }

 #if HIPBLASLT_VERSION_MAJOR > 0 || HIPBLASLT_VERSION_MINOR >= 15
-      if (cfg.scaling_mode < 0 || cfg.scaling_mode >= (int)HIPBLASLT_MATMUL_MATRIX_SCALE_END)


Line length and { at the end of the line are understood but lint does not require duplicate body of If.

Yes, the duplicate was to fix the readability/braces warning, I have restructured it to compute a bool in preprocessor-only branches, then use one if body.

It looks like there is discrepancy here. .clang-format sets line width 100 and this is what IDE uses. So cpplint should be configured accorfingly

ipanfilo · 2026-04-22T03:10:37Z

                                                    zero_centered_gamma, mode, training);
-  } else
+  }
 #endif


Let's get rid of nested ifs. If splitting else-if does not work, better add dummy 'if (false) {' for ROCm instead of 'if (NOrmBAckend...'

Dropped the nested if (!plan) / inner if (Forward) structure. and added the following more clear structure:

CUDA: if (Cudnn) … else if (Forward TE) … else (backward TE) in one #ifndef HIP_PLATFORM_AMD block.
ROCm: if (Forward TE) … else (backward TE) in #else, with mode/training on TE constructors.

ipanfilo · 2026-04-22T03:14:31Z

        return False

-    with te_wheel_file.open("r") as f:
-        for line in f:


It is upstream code. Why change it?

ipanfilo · 2026-04-22T03:30:36Z

-from packaging import version
 from typing import Optional, Tuple

+from packaging import version


Why is it needed?

This is used to check the following

if version.parse(jax.__version__) >= version.parse("0.5.0"): from jax.experimental.custom_partitioning import SdyShardingRule

I will update the codebase to remove all these checks as we no longer support Jax<0.5.0 as mentioned in #547 (comment)

ipanfilo · 2026-04-22T03:33:10Z

+from jax.experimental.custom_partitioning import BATCHING
 from jax.interpreters.mlir import ir
 from jax.sharding import PartitionSpec
-from .misc import is_hip_extension


Does the file have ROCm code?

Yes, jax version checks were added.

ipanfilo · 2026-04-22T03:33:45Z

 import jax.numpy as jnp
 from jax import dtypes, ffi
-from jax.experimental.custom_partitioning import SdyShardingRule, BATCHING
+if version.parse(jax.__version__) >= version.parse("0.5.0"):


IT is not needed, we don;t actually support JAX < 0.5

ipanfilo · 2026-04-22T03:35:46Z

 from flax.core.frozen_dict import FrozenDict

-from ..util import is_hip_extension, get_jnp_float8_e4m3_type, get_jnp_float8_e5m2_type
+import transformer_engine_jax as tejax


All those changes will result in IFU conflicts

ipanfilo · 2026-04-22T03:36:35Z

 from build_tools.te_version import te_version
 from build_tools.jax import setup_jax_extension, install_requirements, test_requirements

-from pybind11.setup_helpers import build_ext as BuildExtension


Why is it needed?

Reverted and added # pylint: disable=ungrouped-imports

Micky774

@VeeraRajasekhar I still have questions from my initial review unanswered.

The main one which @ipanfilo has also asked is: why such divergence? It will be a huge maintenance burden to reconcile against every subsequent IFU, unless NV upstream implements these exact same decisions (afaik they don't?).

Enforcing linting on ROCm-only files is fine, but we have constraints regarding minimizing diff with upstream even if it means letting some things go "un-linted".

Micky774 · 2026-04-23T13:39:04Z


        # Loop through tiles of current MM problem.
-        while tile >= last_mm_tile and tile < last_mm_tile + num_tiles:
+        while last_mm_tile <= tile < last_mm_tile + num_tiles:


Triton doesn't support this multi-comparison -- it looks like the linter is trying to enforce python semantics on it

ipanfilo · 2026-04-24T03:15:10Z

        if (log_dir_str == "1") {
          log_stream = &std::cout;
-        }
-        else if (open_ck_fused_attn_log_file(log_file, "ck_fused_attn", log_dir_str)) {


Move them to single line then but do not create nested ifs

ipanfilo · 2026-04-24T03:20:17Z

      }

 #if HIPBLASLT_VERSION_MAJOR > 0 || HIPBLASLT_VERSION_MINOR >= 15
-      if (cfg.scaling_mode < 0 || cfg.scaling_mode >= (int)HIPBLASLT_MATMUL_MATRIX_SCALE_END)


It looks like there is discrepancy here. .clang-format sets line width 100 and this is what IDE uses. So cpplint should be configured accorfingly

github-actions · 2026-05-07T19:21:09Z

Claude Walkthrough

Intent. Prepare the ROCm TransformerEngine fork for a CI lint gate by fixing the existing pylint and cpplint violations across the AMD/ROCm-touched code paths and tightening the lint configuration. The PR contains no functional changes — every edit is style, formatting, docstrings, or lint-driven C++ idiom cleanup.

Key changes.

Adjust pylintrc disable list — add missing-module-docstring, missing-function-docstring, possibly-used-before-assignment, fixme, unnecessary-lambda-assignment, use-dict-literal, redefined-outer-name, redefined-builtin so the gate is enforceable on the existing codebase. See pylintrc:7-44.
Sweep ~40 Python files across transformer_engine/{common,jax,pytorch} for pylint compliance: add module/function docstrings, reorder imports (stdlib → third-party → local), unwrap long lines, prefix unused params with _, add encoding="utf-8" to open() calls, and rename a few shadowing locals.
Fix cpplint violations in C++/CUDA/HIP sources: replace C-style (int)x casts with static_cast<int>(...) (transformer_engine/common/gemm/rocm_gemm.cu, transformer_engine/common/permutation/permutation.cu, transformer_engine/common/util/logging.h), mark single-arg constructors explicit (transformer_engine/common/gemm/rocm_gemm.cu:120, :464), replace typedef with using, brace single-statement bodies (transformer_engine/common/amd_detail/hip_float8.h:64-68), and switch a function-pointer-to-void* cast to a union to avoid a non-ISO reinterpret_cast (transformer_engine/common/ck_fused_attn/src/ck_fused_attn_utils.cpp:78-84).
Restructure #ifdef blocks in transformer_engine/common/normalization/common.cpp:544-568 and transformer_engine/pytorch/csrc/extensions/gemm.cpp:325-345 so preprocessor branches do not straddle if/else bodies — required for cpplint without changing semantics.
Reformat transformer_engine/pytorch/tensor/fsdp2_allgather_tensor.py (largest delta): docstrings, line wrapping, drop unused Float8Quantizer import, and add typing.cast(tuple[nn.Module, str, bool], meta) in __torch_dispatch__ to satisfy type narrowing. No control-flow changes.

Walkthrough.

pylintrc — Despite the PR title, this adds missing-module-docstring and missing-function-docstring to the disable list because many private helpers don't get docstrings in the diff. Also disables possibly-used-before-assignment and redefined-builtin, both of which fire heavily in kernel/quantizer code (min, max, id as parameter names).
transformer_engine/common/__init__.py — Adds a docstring to is_fp8_fnuz, fixes whitespace, and adds encoding="utf-8" to the ROCm version-check file reads. Renames the lambda parameter f → line in the build_info filter to avoid shadowing the outer file handle.
transformer_engine/common/gemm/rocm_gemm.cu — explicit on ObjCache and NameMapper single-arg constructors. Extracts a scaling_mode_unsupported predicate so the #if HIPBLASLT_VERSION branches no longer split an if (...) head from its body. typedef unsigned long long ServiceStreamKey → using ServiceStreamKey = std::uint64_t.
transformer_engine/common/normalization/common.cpp — Rewrites the cuDNN-vs-TE plan selection so #ifdef NVTE_USE_CUDNN_FRONTEND now wraps the entire if/else chain instead of straddling the else clause. Functionally identical; the AMD (no-cuDNN) branch now passes mode, training directly without a nested #ifdef __HIP_PLATFORM_AMD__.
transformer_engine/common/ck_fused_attn/src/ck_fused_attn_utils.cpp — The dladdr((void*)set_aiter_asm_dir, &info) call relied on a function-pointer→void* conversion that is technically undefined in ISO C++. Replaced with a union { void (*fn)(); void *addr; } reinterpretation, which lints clean and is the POSIX-recommended idiom. Also unflattens an else if into nested if/else.
transformer_engine/jax/util.py — Re-indented 2-space → 4-space (PEP 8), made subprocess.run multi-line with explicit check=False, added a module docstring. The lambda dtype getters are kept (covered by the new unnecessary-lambda-assignment disable).
transformer_engine/pytorch/utils.py — Two lambda dtype getters were converted to real def functions (get_torch_float8_e4m3_type, get_torch_float8_e5m2_type). is_bf16_compatible was flattened to remove redundant if/else and gained a correct -> bool annotation (was -> None).
transformer_engine/pytorch/tensor/fsdp2_allgather_tensor.py — Mostly cosmetic. Two semi-substantive bits: (1) the unused Float8Quantizer import is dropped, and (2) typing.cast(...) is added before the rewrap closure in __torch_dispatch__ so pylint can prove the metadata isn't None. Behavior unchanged.
transformer_engine/pytorch/triton_kernels/cast_transpose.py — Adds # pylint: disable-next=comparison-with-itself near each NaN-check (a != a) since pylint can't recognize the NaN idiom in Triton kernels. Renames the kernel constexpr MXFP8_BLOCK_SCALING_SIZE → _MXFP8_BLOCK_SCALING_SIZE to avoid shadowing the module-level constant.

Testing. No tests added or modified. The PR is lint-only and the author has not flagged a behavior change; correctness rests on the existing CI suite continuing to pass plus visual review of the affected sites.

Notes for reviewers.

No .github/workflows/ file is added despite the PR title — the actual lint workflow is presumably a follow-up. Worth confirming with the author whether a workflow is intended in this PR or a successor.
The two preprocessor restructurings (common/normalization/common.cpp, pytorch/csrc/extensions/gemm.cpp) are the only places where a careful diff read is warranted — both rearrange #if boundaries around if/else bodies. Each branch invokes the same constructor/function with the same arguments before and after.
The union-based function-pointer cast in ck_fused_attn_utils.cpp is well-defined on every supported platform but technically implementation-defined; with -Wpedantic/-Wstrict-aliasing=2 it may still warn.
Renaming MXFP8_BLOCK_SCALING_SIZE → _MXFP8_BLOCK_SCALING_SIZE inside the Triton kernel signature changes the kwarg name visible to Triton's autotuner and any caller that passes by name. A grep confirms the kernel is only invoked positionally inside this file, but worth a glance.
Promoting the dtype lambdas in pytorch/utils.py to defs changes their __name__ / __qualname__ (from <lambda> to the function name). Anything that introspects them (debuggers, tracing) sees the new names.

Generated by Claude. To request a code review, comment `/claude review`.

VeeraRajasekhar self-assigned this Apr 17, 2026

Micky774 reviewed Apr 17, 2026

View reviewed changes

VeeraRajasekhar added 3 commits April 21, 2026 05:55

Addressed review

4ea0d55

CI: Fixed python pylint issues

6748cd8

VeeraRajasekhar marked this pull request as ready for review April 21, 2026 22:47

VeeraRajasekhar requested review from ipanfilo, wangye805 and wenchenvincent as code owners April 21, 2026 22:47

VeeraRajasekhar added the ci-level 3 CI test level 3 label Apr 21, 2026

ipanfilo requested changes Apr 22, 2026

View reviewed changes

Micky774 requested changes Apr 22, 2026

View reviewed changes

Addressed reviews

b60c7f9

Micky774 reviewed Apr 23, 2026

View reviewed changes

ipanfilo requested changes Apr 24, 2026

View reviewed changes

VeeraRajasekhar added 2 commits April 25, 2026 03:08

Addressed reviews

caceac3

Addressed reviews for python files

6bc015c

wenchenvincent added the claude-summary label May 7, 2026

github-actions Bot removed the claude-summary label May 7, 2026

Conversation

VeeraRajasekhar commented Apr 17, 2026

Description

Type of change

Changes

Checklist:

Uh oh!

Micky774 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Micky774 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 7, 2026

Claude Walkthrough

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants