Skip to content

fix(loader): resolve OpenMP symbol through torch - #2936

Merged
zufayu merged 1 commit into
mainfrom
codex/fix-loader-omp-symbol
Nov 3, 2026
Merged

zufayu merged 1 commit into
mainfrom
codex/fix-loader-omp-symbol

Conversation

@zufayu

@zufayu zufayu commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

The ROCm 10.0 Nightly Docker Release fails while testing the image because loader worker initialization resolves omp_set_num_threads from the process-global symbol table. PyTorch may load its OpenMP dependency with RTLD_LOCAL, so ctypes.CDLL(None) raises undefined symbol: omp_set_num_threads and breaks the loader thread pool.

Resolve the symbol through the already-loaded torch._C extension and its dependency graph. This selects the same OpenMP runtime used by PyTorch while preserving the per-loader-thread limit. Add a regression test that verifies the lookup uses the torch extension rather than the global process handle.

Failure evidence: https://github.com/ROCm/ATOM/actions/runs/41456304159

Validation:

  • python -m compileall atom/model_loader/loading_core.py tests/test_loader_threads.py
  • git diff --check
  • PyTorch, Black, and Ruff are unavailable in the local Windows environment; repository CI will run the executable test and formatting checks.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

🏷️ CI Guide

Runs automatically on every eligible PR before approval:

  • ✅ Pre Checkin: Black, Ruff, catalog schema validation, non-GPU unit tests

Heavy model tests:

  • ✅ Run after the PR is approved and Pre Checkin passes
  • ✅ Run immediately when an approval review is submitted
  • ✅ Can be requested before approval with labels
Label Tests
ci:full Run all heavy PR model tests: native ATOM, vLLM, and SGLang
ci:atom Run native ATOM model accuracy tests
ci:vllm Run ATOM vLLM OOT model accuracy tests
ci:sglang Run ATOM SGLang model accuracy tests

Heavy jobs are skipped when the PR is not approved and no matching ci:* label is present.
Add labels via the sidebar or gh pr edit 2936 --add-label <label>

@zufayu
zufayu requested a review from gyohuangxin November 3, 2026 03:33

@valar-cop valar-cop left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

one thing to confirm before merge..

post-fix run in the rocm 10 nightly image showing the loader pool starts?

ci green.

Comment thread atom/model_loader/loading_core.py Resolved
@zufayu zufayu added the ci:atom label Nov 3, 2026
@zufayu

zufayu commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Status: review thread resolved, all required checks green, ready to squash-merge — except the for_main gate needs one code-owner approval (I pushed the branch, so I can't self-approve).

On the inline question — the fix resolves omp_set_num_threads strictly through torch._C's dependency chain, i.e. torch's own bundled OpenMP runtime. If the image preloads libomp separately, that is exactly the runtime torch._C already depends on, so the lookup still selects torch's OMP rather than a second implementation. The definitive confirmation is the ROCm 10.0 nightly (loader pool init on the built image); if it still fails there, we revert.

@gyohuangxin could you take a final look / approve when you get a chance?

@valar-cop

Copy link
Copy Markdown

@zufayu
zufayu merged commit 825b63b into main Nov 3, 2026
85 of 154 checks passed
@zufayu
zufayu deleted the codex/fix-loader-omp-symbol branch November 3, 2026 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants