Skip to content

Commit 4463754

Browse files
committed
fix: Rename variable to control PG timeout to not refer to NCCL
The variable is not backend specific. In the future, if/when we support other backends, this will become more evidently a problem. Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
1 parent b75fd4e commit 4463754

5 files changed

Lines changed: 18 additions & 13 deletions

File tree

README.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,9 @@ run_training(
374374

375375
Below is a list of custom environment variables users can set in the training library.
376376

377-
1. `INSTRUCTLAB_NCCL_TIMEOUT_MS`, this environment variable controls the NCCL timeout in milliseconds. Consider increasing if seeing FSDP related NCCL errors.
377+
1. `INSTRUCTLAB_PROCESS_GROUP_TIMEOUT_MS`, this environment variable controls
378+
the process group timeout in milliseconds. Consider increasing if seeing
379+
FSDP related collective timeout errors.
378380

379381
## Developer Certificate of Origin
380382

src/instructlab/training/const.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
# SPDX-License-Identifier: Apache-2.0
2+
3+
INSTRUCTLAB_PROCESS_GROUP_TIMEOUT_MS = "INSTRUCTLAB_PROCESS_GROUP_TIMEOUT_MS"

src/instructlab/training/main_ds.py

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,7 @@
5252
TorchrunArgs,
5353
TrainingArgs,
5454
)
55-
56-
# pylint: disable=no-name-in-module
55+
from instructlab.training.const import INSTRUCTLAB_PROCESS_GROUP_TIMEOUT_MS
5756
from instructlab.training.logger import (
5857
propagate_package_logs,
5958
setup_metric_logger,
@@ -278,7 +277,7 @@ def train(
278277
# time of writing) default: to cover the unlikely event torch decides to tweak
279278
# the default.
280279
def _get_collective_timeout() -> datetime.timedelta | None:
281-
timeout_var = os.getenv("INSTRUCTLAB_NCCL_TIMEOUT_MS")
280+
timeout_var = os.getenv(INSTRUCTLAB_PROCESS_GROUP_TIMEOUT_MS)
282281
if timeout_var is None:
283282
return None
284283

@@ -289,7 +288,7 @@ def _get_collective_timeout() -> datetime.timedelta | None:
289288

290289
if timeout <= 0:
291290
raise ValueError(
292-
f"Invalid value for INSTRUCTLAB_NCCL_TIMEOUT_MS: {timeout_var}. Must be a positive integer."
291+
f"Invalid value for {INSTRUCTLAB_PROCESS_GROUP_TIMEOUT_MS}: {timeout_var}. Must be a positive integer."
293292
)
294293

295294
return datetime.timedelta(milliseconds=timeout)

tests/unit/test_main_ds.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
# First Party
99
from instructlab.training import main_ds
10+
from instructlab.training.const import INSTRUCTLAB_PROCESS_GROUP_TIMEOUT_MS
1011

1112

1213
def test__get_collective_timeout():
@@ -16,7 +17,7 @@ def test__get_collective_timeout():
1617
# Test with custom timeout
1718
timeout = 1234
1819
with mock.patch.dict(
19-
main_ds.os.environ, {"INSTRUCTLAB_NCCL_TIMEOUT_MS": str(timeout)}
20+
main_ds.os.environ, {INSTRUCTLAB_PROCESS_GROUP_TIMEOUT_MS: str(timeout)}
2021
):
2122
assert main_ds._get_collective_timeout() == datetime.timedelta(
2223
milliseconds=timeout
@@ -25,15 +26,15 @@ def test__get_collective_timeout():
2526
# Test with invalid timeout (negative)
2627
invalid_timeout = "-100"
2728
with mock.patch.dict(
28-
main_ds.os.environ, {"INSTRUCTLAB_NCCL_TIMEOUT_MS": invalid_timeout}
29+
main_ds.os.environ, {INSTRUCTLAB_PROCESS_GROUP_TIMEOUT_MS: invalid_timeout}
2930
):
3031
with pytest.raises(ValueError):
3132
main_ds._get_collective_timeout()
3233

3334
# Test with invalid timeout (string)
3435
invalid_timeout = "invalid"
3536
with mock.patch.dict(
36-
main_ds.os.environ, {"INSTRUCTLAB_NCCL_TIMEOUT_MS": invalid_timeout}
37+
main_ds.os.environ, {INSTRUCTLAB_PROCESS_GROUP_TIMEOUT_MS: invalid_timeout}
3738
):
3839
with pytest.raises(ValueError):
3940
main_ds._get_collective_timeout()

tox.ini

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,9 @@ basepython = python3.11
2020
[testenv:py3-unit]
2121
description = run unit tests with pytest
2222
passenv =
23-
HF_HOME
24-
INSTRUCTLAB_NCCL_TIMEOUT_MS
25-
CMAKE_ARGS
23+
HF_HOME
24+
INSTRUCTLAB_PROCESS_GROUP_TIMEOUT_MS
25+
CMAKE_ARGS
2626

2727
# Use PyTorch CPU build instead of CUDA build in test envs. CUDA dependencies
2828
# are huge. This reduces venv from 5.7 GB to 1.5 GB.
@@ -40,8 +40,8 @@ commands = {envpython} -m pytest tests/unit {posargs}
4040
[testenv:py3-smoke]
4141
description = run accelerated smoke tests with pytest
4242
passenv =
43-
HF_HOME
44-
INSTRUCTLAB_NCCL_TIMEOUT_MS
43+
HF_HOME
44+
INSTRUCTLAB_PROCESS_GROUP_TIMEOUT_MS
4545
deps =
4646
-r requirements-dev.txt
4747
-r requirements-cuda.txt

0 commit comments

Comments
 (0)