Skip to content

fix(ascend): correct rotary embedding handling - #953

Merged
voltjia merged 1 commit into
masterfrom
fix/ascend-rotary-embedding
Aug 26, 2026
Merged

fix(ascend): correct rotary embedding handling#953
voltjia merged 1 commit into
masterfrom
fix/ascend-rotary-embedding

Conversation

@baominghelly

@baominghelly baominghelly commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Correct the Ascend RotaryEmbedding implementation to process query and optional key tensors independently with aclnnRotaryPositionEmbedding, then copy the results back to preserve in-place behavior.
  • Extend tests/test_rotary_embedding.py to cover Ascend's supported forward full-dimension NeoX configuration for FP32, FP16, and BF16 with and without a key tensor.

Motivation

The Ascend implementation on master requires a key tensor and uses a joint rotary-position API/layout that does not preserve the expected full-dimension NeoX values. This also prevents the query-only execution permitted by the shared operator interface.

Type of Change

  • feat — new feature / new operator / new platform
  • fix — bug fix
  • perf — performance improvement (no behavioral change)
  • refactor — code restructuring without behavior change
  • test — adding or fixing tests only
  • docs — documentation only
  • build / ci — build system or CI configuration
  • chore — tooling, formatting, or other non-code changes
  • Breaking change (requires a ! in the Conventional Commits prefix or a BREAKING CHANGE: footer)

Platforms Affected

  • CPU (WITH_CPU)
  • NVIDIA (WITH_NVIDIA)
  • Iluvatar (WITH_ILUVATAR)
  • MetaX (WITH_METAX)
  • Cambricon (WITH_CAMBRICON)
  • Moore (WITH_MOORE)
  • Ascend (WITH_ASCEND)
  • PyTorch C++ bindings (WITH_TORCH)
  • Build system / CMake / CI
  • Python bindings / user-facing API

Smoke Test Result

Environment:
  Hardware: Ascend 910C
  CANN: 9.0.0
  SOC_VERSION: Ascend910_9382
  InfiniRT prefix: /workspace/InfiniRT-6b256e0-prefix
  InfiniOps head: 1101f851ace94b474174761813130850cda162ce
  Source tree: d57e5fe69c23c03e3e34aee2eb2e5e6bd30470f7

$ cmake -S "$SRC" -B "$BUILD" \
    -DCMAKE_BUILD_TYPE=Release \
    -DPython_EXECUTABLE=/usr/local/python3.11.15/bin/python \
    -DWITH_CPU=ON \
    -DWITH_ASCEND=ON \
    -DWITH_NVIDIA=OFF \
    -DAUTO_DETECT_DEVICES=OFF \
    -DAUTO_DETECT_BACKENDS=OFF \
    -DGENERATE_OPERATOR_CALL_INSTANTIATIONS=ON \
    -DGENERATE_PYTHON_BINDINGS=ON \
    -DINFINI_OPS_OPS=rotary_embedding \
    -DINFINI_RT_ROOT=/workspace/InfiniRT-6b256e0-prefix \
    -DSOC_VERSION=Ascend910_9382
$ cmake --build "$BUILD" --target ops -j32
[ 55%] Built target infiniops
[100%] Built target ops
$ cmake --install "$BUILD" --prefix "$INSTALL_ROOT/infini"
-- Installing: .../infini/ops.cpython-311-aarch64-linux-gnu.so

$ python -m pytest -q tests/test_rotary_embedding.py --devices ascend
.sss.sss.sss.sss.sss.sss
6 passed, 18 skipped, 14 warnings in 1.26s

$ ruff check tests/test_rotary_embedding.py
All checks passed!
$ ruff format --check tests/test_rotary_embedding.py
1 file already formatted

Test Results on Supported Platforms

Platform Affected Build / Smoke Result Full Result / Notes
NVIDIA No N/A - not affected N/A - not affected
Iluvatar No N/A - not affected N/A - not affected
MetaX No N/A - not affected N/A - not affected
Cambricon No N/A - not affected N/A - not affected
Moore No N/A - not affected N/A - not affected
Ascend Yes targeted operator build passed RoPE targeted suite: 6 passed, 18 skipped; full suite not run
Full `pytest` output
.sss.sss.sss.sss.sss.sss                                                 [100%]

SKIPPED [18]: Ascend supports forward full-dimension NeoX RoPE

6 passed, 18 skipped, 14 warnings in 1.26s

Benchmark / Performance Impact

N/A. This is a correctness fix; no performance benchmark was collected.

Notes for Reviewers

  • The change is limited to the Ascend implementation and its regression test; it does not modify the shared operator interface or other backend implementations.
  • Ascend currently supports forward full-dimension NeoX RoPE. Interleaved, partial-dimension, and inverse variants remain explicit test skips.
  • Query and optional key tensors are executed independently and copied back to preserve the existing in-place contract.
  • The final commit amended only author metadata after the build. The tested source tree remained d57e5fe69c23c03e3e34aee2eb2e5e6bd30470f7, and the targeted test was rerun on the final branch.
  • clang-format was unavailable in the CANN development container; formatting is deferred to CI. git diff --check, Ruff 0.15.22, the targeted build, and the targeted Ascend test passed.

@baominghelly
baominghelly marked this pull request as ready for review August 26, 2026 03:18
@baominghelly
baominghelly requested review from a team and voltjia August 26, 2026 03:18
@voltjia
voltjia merged commit 06cfc3d into master Aug 26, 2026
16 of 20 checks passed
@voltjia
voltjia deleted the fix/ascend-rotary-embedding branch August 26, 2026 03:20
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.

2 participants