[DISCUSSION] Should safe_get_rank() warn or raise on invalid RANK values instead of falling back to 0?
- Dominant language
- Python
- Stars
- 17.9k
- Forks
- 4.5k
- Avg merge
- 4d 6h
- Merged PRs (30d)
- 271
Description
**Describe the bug / discussion topic**
@NVIDIA/mcore-oncall
During review on PR #4959, a question came up about whether `safe_get_rank()` should continue to silently fall back to `0` when `RANK` is present but not parseable as an integer.
Today, both helpers keep longstanding fallback semantics:
- `megatron/core/_rank_utils.py`
- `megatron/core/distributed/fsdp/src/megatron_fsdp/utils.py`
Both currently catch `ValueError` / `TypeError` and return `0`.
This issue is to discuss whether that should remain the desired behavior, or whether invalid `RANK` values should instead surface a warning or raise an error.
**Steps/Code to reproduce bug**
Current behavior with a malformed environment variable is effectively:
```python
import os
os.environ["RANK"] = "not-an-int"
# safe_get_rank() currently returns 0
```
This happens because both implementations catch `ValueError` / `TypeError` from `int(...)` conversion and default to `0`.
**Expected behavior**
It would be useful to clarify the intended behavior for malformed `RANK` values. Reasonable options seem to be:
- keep current behavior: fall back to `0`
- warn, then fall back to `0`
- raise `ValueError` to surface misconfiguration immediately
**Additional context**
- This behavior is longstanding in the canonical helper: `safe_get_rank()` was introduced in `megatron/core/_rank_utils.py` in commit `6cf285b23` with the current fallback-to-`0` semantics.
- The Megatron-FSDP helper was added later in commit `b969f7642` and copied the same behavior.
- PR #4959 intentionally fixes only the missing `os` import in the FSDP helper and does not change these semantics.
If maintainers prefer a stricter or more explicit behavior here, I can send a follow-up PR to update both helpers consistently.
Contributor guide
Assessment
This issue has not been assessed yet.