Skip to content

fix spurious recompilation on incremental editable builds#669

Open
matthiasdiener wants to merge 1 commit into
devfrom
mdiener/build-incremental-fixes
Open

fix spurious recompilation on incremental editable builds#669
matthiasdiener wants to merge 1 commit into
devfrom
mdiener/build-incremental-fixes

Conversation

@matthiasdiener

@matthiasdiener matthiasdiener commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Description

Two unrelated causes made pip install -e . recompile from scratch even
with no source changes:

  • PyTorch extension: setuptools' editable_wheel points build_ext.build_temp
    at an ephemeral /tmp/*.build-temp dir, so all csrc objects recompiled every
    build. Override finalize_options() to pin build_temp to build/torch_temp,
    giving ninja a persistent object cache.

  • CK grouped gemm: ck_grouped_gemm_common.h had a malformed relative include
    ("../../common/util/system.h", which resolves to a nonexistent path). This
    forced hipify's output-dir-dependent fallback resolution, so the pytorch and
    common hipify passes emitted different include paths and the generated
    _hip.h flip-flopped every build, retriggering all 8 CK objects. Correct the
    include to "../../util/system.h" so both passes agree.

Companion to #666 and Micky774/QoLA#9.
With all three PRs, total rebuild time without source code changes is reduced from ~8 minutes to ~6 seconds, with zero recompilations.

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

Comment thread build_tools/build_ext.py
# scratch. Rooting it at a stable location lets the underlying ninja skip
# unchanged objects. Mirrors the persistent CMake build dir above.
root_dir = Path(__file__).resolve().parent.parent
build_temp = root_dir / "build" / "torch_temp"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit: naming — torch_temp reads as PyTorch-specific, but get_build_ext() is also called from transformer_engine/jax/setup.py:60, so JAX ext objects will land under build/torch_temp/ too. Since both frameworks share this dir, a framework-neutral name like ext_temp (or framework_temp) would be less confusing when someone stumbles into it later. No functional impact — ninja tracks objects by source path so pytorch/jax objects won't collide.

@github-actions

Copy link
Copy Markdown

Reviewed the two-file diff. Both fixes look correct and targeted:

  • build_tools/build_ext.py: finalize_options override pinning build_temp to build/torch_temp correctly addresses the ephemeral setuptools editable_wheel path. build/ is already gitignored. Left one non-blocking nit on the directory name (used by JAX ext too).
  • ck_grouped_gemm_common.h: Include path fix is verified — transformer_engine/common/util/system.h exists; the previous ../../common/util/system.h resolved to a nonexistent transformer_engine/common/common/util/system.h. Note: the file now includes the same header via two paths (line 19 common/util/system.h and line 21 ../../util/system.h) but that's harmless with #pragma once and matches the existing dual-style pattern in the file.

Copyright headers: OK (both files have current AMD end-year 2026; NVIDIA line preserved).

Verdict: LGTM, already approved by @Micky774.

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

Labels

ci-level 1 CI test level 1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants