NVIDIA / NVIDIA/Megatron-LM

[DISCUSSION] Should safe_get_rank() warn or raise on invalid RANK values instead of falling back to 0?

Open
#4,991 1 comment 0 reactions 1 assignee Claimed by @wujingyue View on GitHub
community-request question waiting-on-maintainers
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.