Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix configuration validation error message in Lite CLI #16334

Merged
merged 6 commits into from
Jan 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/lightning_fabric/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).

- Restored sampling parity between PyTorch and Fabric dataloaders when using the `DistributedSampler` ([#16101](https://github.com/Lightning-AI/lightning/issues/16101))

- Fixes an issue where the error message wouldn't tell the user the real value that was passed through the CLI ([#16334](https://github.com/Lightning-AI/lightning/issues/16334))


## [1.8.6] - 2022-12-21

Expand Down
2 changes: 1 addition & 1 deletion src/lightning_fabric/connector.py
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ def _argument_from_env(name: str, current: Any, default: Any) -> Any:
if env_value is not None and env_value != str(current) and str(current) != str(default):
raise ValueError(
f"Your code has `Fabric({name}={current!r}, ...)` but it conflicts with the value "
f"`--{name}={current}` set through the CLI. "
f"`--{name}={env_value}` set through the CLI. "
carmocca marked this conversation as resolved.
Show resolved Hide resolved
" Remove it either from the CLI or from the Lightning Fabric object."
)
if env_value is None:
Expand Down
15 changes: 5 additions & 10 deletions tests/tests_fabric/test_connector.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
# limitations under the License

import os
from re import escape
from typing import Any, Dict
from unittest import mock

Expand Down Expand Up @@ -808,27 +807,23 @@ def test_devices_from_environment(*_):
def test_arguments_from_environment_collision():
"""Test that the connector raises an error when the CLI settings conflict with settings in the code."""
with mock.patch.dict(os.environ, {"LT_ACCELERATOR": "cpu"}):
with pytest.raises(
ValueError, match=escape("Your code has `Fabric(accelerator='cuda', ...)` but it conflicts")
):
with pytest.raises(ValueError, match="`Fabric\\(accelerator='cuda', ...\\)` but .* `--accelerator=cpu`"):
_Connector(accelerator="cuda")

with mock.patch.dict(os.environ, {"LT_STRATEGY": "ddp"}):
with pytest.raises(
ValueError, match=escape("Your code has `Fabric(strategy='ddp_spawn', ...)` but it conflicts")
):
with pytest.raises(ValueError, match="`Fabric\\(strategy='ddp_spawn', ...\\)` but .* `--strategy=ddp`"):
_Connector(strategy="ddp_spawn")

with mock.patch.dict(os.environ, {"LT_DEVICES": "2"}):
with pytest.raises(ValueError, match=escape("Your code has `Fabric(devices=3, ...)` but it conflicts")):
with pytest.raises(ValueError, match="`Fabric\\(devices=3, ...\\)` but .* `--devices=2`"):
_Connector(devices=3)

with mock.patch.dict(os.environ, {"LT_NUM_NODES": "3"}):
with pytest.raises(ValueError, match=escape("Your code has `Fabric(num_nodes=2, ...)` but it conflicts")):
with pytest.raises(ValueError, match="`Fabric\\(num_nodes=2, ...\\)` but .* `--num_nodes=3`"):
_Connector(num_nodes=2)

with mock.patch.dict(os.environ, {"LT_PRECISION": "16"}):
with pytest.raises(ValueError, match=escape("Your code has `Fabric(precision=64, ...)` but it conflicts")):
with pytest.raises(ValueError, match="`Fabric\\(precision=64, ...\\)` but .* `--precision=16`"):
_Connector(precision=64)


Expand Down