Skip to content

fix(ci): use subprocess for Triton compile helpers - #28775

Merged
tianleiwu merged 3 commits into
mainfrom
tlwu/20260603/icm_4
Jun 4, 2026
Merged

fix(ci): use subprocess for Triton compile helpers#28775
tianleiwu merged 3 commits into
mainfrom
tlwu/20260603/icm_4

Conversation

@tianleiwu

Copy link
Copy Markdown
Contributor

Summary

This PR replaces shell-based command execution in the Triton compile helper with subprocess.run(), and adds regression tests to lock in that behavior.

Key Changes

  • Switched tools/ci_build/compile_triton.py to use subprocess.run() for objcopy and ar execution instead of os.system().
  • Added tools/ci_build/test_compile_triton.py to verify the helper functions invoke subprocess-based command execution and preserve the expected working directory and error handling.
  • Hardened directory handling in the Triton compile path so the output directory is created safely with os.makedirs(..., exist_ok=True).

Why

The previous shell-based implementation was fragile and harder to test. Using subprocess.run() makes the command execution path explicit, easier to validate, and safer for CI and cross-platform invocation.

Testing

  • source .venv/bin/activate && python -m unittest tools.ci_build.test_compile_triton
  • source .venv/bin/activate && lintrunner -a

Comment thread tools/ci_build/test_compile_triton.py Fixed
…ith 'import' and 'import from''

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

Copilot AI left a comment

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.

Pull request overview

This PR updates the Triton AoT compile helper to execute external tooling (objcopy, ar) via subprocess.run() instead of shelling out with os.system(), and adds unit tests intended to regression-test that behavior in CI.

Changes:

  • Replaced shell-based objcopy/ar invocations with subprocess.run(..., cwd=..., check=True) in compile_triton.py.
  • Added a new unittest module to validate subprocess.run usage (and that os.system is not used).
  • Hardened output directory handling for --obj_file paths with no directory component.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
tools/ci_build/compile_triton.py Switches external command execution to subprocess.run() and tweaks output directory creation.
tools/ci_build/test_compile_triton.py Adds regression tests to validate subprocess-based command execution behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tools/ci_build/test_compile_triton.py
Comment thread tools/ci_build/test_compile_triton.py
Comment thread tools/ci_build/test_compile_triton.py
Comment thread tools/ci_build/compile_triton.py Outdated
Comment thread tools/ci_build/compile_triton.py Outdated
Comment thread tools/ci_build/compile_triton.py Outdated

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

Comment thread tools/ci_build/test_compile_triton.py
Comment thread tools/ci_build/test_compile_triton.py
Comment thread tools/ci_build/test_compile_triton.py
Comment thread tools/ci_build/compile_triton.py
@tianleiwu
tianleiwu merged commit 368634e into main Jun 4, 2026
87 checks passed
@tianleiwu
tianleiwu deleted the tlwu/20260603/icm_4 branch June 4, 2026 15:28
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.

4 participants