Skip to content

fix(utils): treat a blank or non-numeric LOCAL_RANK as unset - #10246

Merged
hjh0119 merged 1 commit into
modelscope:mainfrom
Lesereingrape:fix/blank-local-rank-import-crash
Sep 28, 2026
Merged

hjh0119 merged 1 commit into
modelscope:mainfrom
Lesereingrape:fix/blank-local-rank-import-crash

Conversation

@Lesereingrape

Copy link
Copy Markdown
Contributor

PR type

  • Bug Fix
  • New Feature
  • Document Updates
  • More Models or Datasets Support

PR information

swift/utils/logger.py read LOCAL_RANK with a bare int() in two places:

  • _is_local_master() — reached while the module is still being imported, because logger = get_logger() runs at import time and get_logger() calls _is_local_master();
  • add_file_handler_if_needed() — the same read, guarded by the find_spec('torch') check.

So a LOCAL_RANK that is present but not a number raises ValueError from the import itself:

$ LOCAL_RANK= python -c "from swift.utils import get_logger"
  File ".../swift/utils/logger.py", line 121, in <module>
  File ".../swift/utils/logger.py", line 96, in get_logger
  File ".../swift/utils/logger.py", line 13, in _is_local_master
ValueError: invalid literal for int() with base 10: ''

LOCAL_RANK= (declared but left empty) is what a launcher script or a Dockerfile produces when the variable is exported before the value is known — ENV LOCAL_RANK= in an image, or export LOCAL_RANK= in a wrapper that only fills it in for the distributed path. Today that makes swift.utils unimportable, which takes down every entry point behind it. Note that int() already tolerates an unset variable through its -1 default, so the crash is specific to a value that exists but does not parse.

This PR adds one _get_local_rank() helper that falls back to the unset default (-1, i.e. local master) when the value does not parse, and routes both sites through it. A rank that does parse keeps its exact meaning, so rank > 0 still silences the logger — the fallback only covers the case that previously raised. This mirrors how LOG_LEVEL is resolved right below it (_get_log_level() falls back to INFO rather than raising), which is the same "a blank env var should not break the import" argument that #10245 merged for.

Why the read is duplicated in this file at all: swift/utils/env.py already has get_dist_setting() / is_local_master(), but env.py:9 does from .logger import get_logger, so logger.py cannot import it back — that is what the existing # Avoid circular reference comment marks. This PR keeps the fallback local to logger.py for the same reason, and deliberately does not touch env.py, whose reads fail at call time rather than at import and therefore need their own discussion.

Experiment results

All numbers below were produced on this checkout with the project's own runner (python -m unittest, since tests/run.py discovers tests/**/test_*.py), Python 3.12 on Windows, against main @ c08110b.

New test tests/utils/test_local_rank.py, 7 cases, each in a fresh subprocess so the import-time behaviour is what is measured:

  • RED — pristine main logger.py + the new test: Ran 7 tests ... FAILED (failures=4). The four failures are the blank, whitespace, non-numeric and file-handler cases; the three controls (unset, LOCAL_RANK=0, LOCAL_RANK=1) pass, so the test does not simply assert the crash away.
  • GREEN — with the fix: Ran 7 tests ... OK.
  • Both sites are independently live. With only the import-time site fixed and the add_file_handler_if_needed read left as a bare int(), the blank value still raises:
    File ".../swift/utils/logger.py", line 88, in get_logger
    File ".../swift/utils/logger.py", line 163, in add_file_handler_if_needed
    ValueError: invalid literal for int() with base 10: ''
    
    Routing both through the helper is what closes it.
  • tests/utils regression, measured by swapping logger.py between this branch and pristine main on the same venv: with the fix Ran 154 tests ... FAILED (failures=2, errors=7, skipped=22); with pristine logger.py and the same test file present Ran 154 tests ... FAILED (failures=6, errors=7, skipped=22). The delta is exactly the 4 new cases above, and the remaining 2 failures plus all 7 errors and 22 skips are identical in both runs — they come from this minimal environment missing optional dependencies, not from the change.
  • Gate: pre-commit run --files swift/utils/logger.py tests/utils/test_local_rank.py → flake8 / isort / yapf / trailing-whitespace / end-of-file / double-quote-string / mixed-line-ending all Passed.

Diff is +12/-3 in swift/utils/logger.py plus the new test file.

A note on how this patch was prepared: it was written and measured by an automated agent session working for the account owner. The numbers above are its own measurements, reproduced on main and on this branch immediately before opening the PR; the owner has not independently re-run them.

Logging is configured while swift.utils.logger is imported, so the two bare
int(os.getenv('LOCAL_RANK', -1)) reads raise ValueError from the import itself
when the variable is present but does not parse (e.g. `LOCAL_RANK=` left in a
launcher script or a Dockerfile). Route both through one helper that falls back
to the unset default, so a rank that parses keeps its meaning.
@hjh0119
hjh0119 merged commit 3efd1f8 into modelscope:main Sep 28, 2026
3 checks passed
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