From 1384c12323d9b1d7c451d59396b177b3eac31cf8 Mon Sep 17 00:00:00 2001 From: Victor Skvortsov Date: Wed, 5 Aug 2026 09:32:11 +0500 Subject: [PATCH 1/2] Fix extra run/fleet configuration properties not ignored when parsing --- src/dstack/_internal/core/models/fleets.py | 19 ++--- src/dstack/_internal/core/models/runs.py | 22 ++---- .../_internal/core/models/test_fleets.py | 78 ++++++++++++++++++- src/tests/_internal/core/models/test_runs.py | 68 +++++++++++++++- 4 files changed, 157 insertions(+), 30 deletions(-) diff --git a/src/dstack/_internal/core/models/fleets.py b/src/dstack/_internal/core/models/fleets.py index 963edbfd5..07911666a 100644 --- a/src/dstack/_internal/core/models/fleets.py +++ b/src/dstack/_internal/core/models/fleets.py @@ -384,26 +384,19 @@ class FleetSpec(CoreModel): Read profile parameters from `merged_profile` instead of `profile` directly. """ - @model_validator(mode="before") - @classmethod - def _merged_profile(cls, values) -> Dict: - try: - # Copy first: `model_validate` returns the *same* instance for a same-class input, so - # the `setattr` loop below would otherwise mutate the caller's profile in place. - merged_profile = Profile.model_validate(values["profile"]).model_copy(deep=True) - conf = FleetConfiguration.model_validate(values["configuration"]) - except KeyError: - raise ValueError("Missing profile or configuration") + @model_validator(mode="after") + def _merged_profile(self) -> Self: + merged_profile = self.profile.model_copy(deep=True) for key in ProfileParams.model_fields: - conf_val = getattr(conf, key, None) + conf_val = getattr(self.configuration, key, None) if conf_val is not None: setattr(merged_profile, key, conf_val) if merged_profile.spot_policy is None: merged_profile.spot_policy = SpotPolicy.ONDEMAND if merged_profile.retry is None: merged_profile.retry = False - values["merged_profile"] = merged_profile - return values + self.merged_profile = merged_profile + return self class Fleet(CoreModel): diff --git a/src/dstack/_internal/core/models/runs.py b/src/dstack/_internal/core/models/runs.py index a292a928b..9951696a4 100644 --- a/src/dstack/_internal/core/models/runs.py +++ b/src/dstack/_internal/core/models/runs.py @@ -24,7 +24,6 @@ HTTPHeaderSpec, HTTPMethod, RepoExistsAction, - RunConfiguration, ServiceConfiguration, ) from dstack._internal.core.models.files import FileArchiveMapping @@ -588,27 +587,20 @@ class RunSpec(CoreModel): Read profile parameters from `merged_profile` instead of `profile` directly. """ - @model_validator(mode="before") - @classmethod - def _merged_profile(cls, values) -> Dict: - if values.get("profile") is None: + @model_validator(mode="after") + def _merged_profile(self) -> Self: + if self.profile is None: merged_profile = Profile(name="default") else: - # Copy first: `model_validate` returns the *same* instance for a same-class input, so - # the `setattr` loop below would otherwise mutate the caller's profile in place. - merged_profile = Profile.model_validate(values["profile"]).model_copy(deep=True) - try: - conf = RunConfiguration.model_validate(values["configuration"]).root - except KeyError: - raise ValueError("Missing configuration") + merged_profile = self.profile.model_copy(deep=True) for key in ProfileParams.model_fields: - conf_val = getattr(conf, key, None) + conf_val = getattr(self.configuration, key, None) if conf_val is not None: setattr(merged_profile, key, conf_val) if merged_profile.creation_policy is None: merged_profile.creation_policy = CreationPolicy.REUSE_OR_CREATE - values["merged_profile"] = merged_profile - return values + self.merged_profile = merged_profile + return self @model_validator(mode="after") def _validate_dynamo_no_retry(self) -> Self: diff --git a/src/tests/_internal/core/models/test_fleets.py b/src/tests/_internal/core/models/test_fleets.py index 83be0cabb..ee13f8e4b 100644 --- a/src/tests/_internal/core/models/test_fleets.py +++ b/src/tests/_internal/core/models/test_fleets.py @@ -3,7 +3,9 @@ import pytest from pydantic import ValidationError -from dstack._internal.core.models.fleets import FleetConfiguration, FleetNodesSpec +from dstack._internal.core.models.common import validate_extra_ignore +from dstack._internal.core.models.fleets import FleetConfiguration, FleetNodesSpec, FleetSpec +from dstack._internal.core.models.profiles import Profile, SpotPolicy class TestFleetConfiguration: @@ -120,3 +122,77 @@ def test_rejects_nodes(self, input_nodes: Any): } with pytest.raises(ValidationError): FleetConfiguration.model_validate(configuration_input) + + +class TestFleetSpec: + @pytest.mark.parametrize( + "spec", + [ + pytest.param( + { + "configuration": {"type": "fleet", "new_prop": 1}, + "profile": {"name": "default"}, + }, + id="configuration", + ), + pytest.param( + { + "configuration": {"type": "fleet"}, + "profile": {"name": "default", "new_prop": 1}, + }, + id="profile", + ), + pytest.param( + { + "configuration": { + "type": "fleet", + "resources": {"gpu": {"name": ["A100"], "new_prop": 1}}, + }, + "profile": {"name": "default"}, + }, + id="nested-in-configuration", + ), + pytest.param( + { + "configuration": {"type": "fleet"}, + "profile": {"name": "default"}, + "new_prop": 1, + }, + id="top-level", + ), + ], + ) + def test_extra_ignored_on_read_path(self, spec: dict): + validate_extra_ignore(FleetSpec, spec) + with pytest.raises(ValidationError, match="new_prop"): + FleetSpec.model_validate(spec) + + def test_configuration_profile_params_override_profile(self): + spec = FleetSpec.model_validate( + { + "configuration": {"type": "fleet", "reservation": "conf-reservation"}, + "profile": {"name": "default", "reservation": "profile-reservation"}, + } + ) + assert spec.merged_profile.reservation == "conf-reservation" + assert spec.merged_profile.spot_policy == SpotPolicy.ONDEMAND + assert spec.merged_profile.retry is False + + def test_merging_does_not_mutate_the_passed_profile(self): + profile = Profile(name="default", spot_policy=SpotPolicy.ONDEMAND) + spec = FleetSpec.model_validate( + {"configuration": {"type": "fleet", "spot_policy": "spot"}, "profile": profile} + ) + assert spec.merged_profile.spot_policy == SpotPolicy.SPOT + assert profile.spot_policy == SpotPolicy.ONDEMAND + + @pytest.mark.parametrize( + ["spec", "missing_field"], + [ + pytest.param({"configuration": {"type": "fleet"}}, "profile", id="missing-profile"), + pytest.param({"profile": {"name": "default"}}, "configuration", id="missing-conf"), + ], + ) + def test_missing_required_fields_rejected(self, spec: dict, missing_field: str): + with pytest.raises(ValidationError, match=missing_field): + validate_extra_ignore(FleetSpec, spec) diff --git a/src/tests/_internal/core/models/test_runs.py b/src/tests/_internal/core/models/test_runs.py index e0bb9fbce..6380f4694 100644 --- a/src/tests/_internal/core/models/test_runs.py +++ b/src/tests/_internal/core/models/test_runs.py @@ -2,12 +2,18 @@ from pydantic import ValidationError from dstack._internal.core.compatibility.runs import get_run_spec_excludes +from dstack._internal.core.models.common import validate_extra_ignore from dstack._internal.core.models.configurations import ( DevEnvironmentConfiguration, ServiceConfiguration, TaskConfiguration, ) -from dstack._internal.core.models.profiles import RetryEvent +from dstack._internal.core.models.profiles import ( + CreationPolicy, + Profile, + RetryEvent, + SpotPolicy, +) from dstack._internal.core.models.runs import ( JobStatus, JobTerminationReason, @@ -185,3 +191,63 @@ def test_non_service_run_with_retry_is_accepted(self): "repo_data": {"repo_type": "virtual"}, } RunSpec.model_validate(spec) + + +class TestRunSpec: + @pytest.mark.parametrize( + "spec", + [ + pytest.param( + {"configuration": {"type": "dev-environment", "new_prop": 1}}, id="configuration" + ), + pytest.param( + { + "configuration": {"type": "dev-environment"}, + "profile": {"name": "default", "new_prop": 1}, + }, + id="profile", + ), + pytest.param( + { + "configuration": { + "type": "dev-environment", + "resources": {"gpu": {"name": ["A100"], "new_prop": 1}}, + } + }, + id="nested-in-configuration", + ), + pytest.param( + {"configuration": {"type": "dev-environment"}, "new_prop": 1}, id="top-level" + ), + ], + ) + def test_extra_ignored_on_read_path(self, spec: dict): + validate_extra_ignore(RunSpec, spec) + with pytest.raises(ValidationError, match="new_prop"): + RunSpec.model_validate(spec) + + def test_configuration_profile_params_override_profile(self): + spec = RunSpec.model_validate( + { + "configuration": {"type": "dev-environment", "spot_policy": "spot"}, + "profile": {"name": "default", "spot_policy": "on-demand", "max_price": 1.5}, + } + ) + assert spec.merged_profile.spot_policy == SpotPolicy.SPOT + assert spec.merged_profile.max_price == 1.5 + assert spec.merged_profile.creation_policy == CreationPolicy.REUSE_OR_CREATE + + def test_merging_does_not_mutate_the_passed_profile(self): + profile = Profile(name="default", spot_policy=SpotPolicy.ONDEMAND) + spec = RunSpec.model_validate( + { + "configuration": {"type": "dev-environment", "spot_policy": "spot"}, + "profile": profile, + } + ) + assert spec.merged_profile.spot_policy == SpotPolicy.SPOT + assert profile.spot_policy == SpotPolicy.ONDEMAND + + def test_missing_configuration_rejected(self): + with pytest.raises(ValidationError, match="configuration"): + validate_extra_ignore(RunSpec, {}) From 1ebdbc04fe9a79e86d0d3f986733c1d9bf5aa4c1 Mon Sep 17 00:00:00 2001 From: Victor Skvortsov Date: Wed, 5 Aug 2026 09:55:51 +0500 Subject: [PATCH 2/2] Turn fill_data model validators into after validators The Pydantic v2 migration mapped these post root_validators to `mode="before"`, so `fill_data` began receiving raw input. A malformed `filename` reached `open()` as-is, surfacing as TypeError/AttributeError instead of a validation error. --- .../_internal/core/backends/base/models.py | 25 ++++-- .../_internal/core/backends/gcp/models.py | 8 +- .../core/backends/kubernetes/models.py | 10 +-- .../_internal/core/backends/nebius/models.py | 20 ++--- .../_internal/core/backends/slurm/models.py | 8 +- .../core/backends/base/test_models.py | 83 +++++++++++++++++++ 6 files changed, 122 insertions(+), 32 deletions(-) create mode 100644 src/tests/_internal/core/backends/base/test_models.py diff --git a/src/dstack/_internal/core/backends/base/models.py b/src/dstack/_internal/core/backends/base/models.py index b65024c1b..f56a6a4a0 100644 --- a/src/dstack/_internal/core/backends/base/models.py +++ b/src/dstack/_internal/core/backends/base/models.py @@ -1,5 +1,5 @@ from pathlib import Path -from typing import List +from typing import List, TypeVar from dstack._internal.core.models.common import CoreModel from dstack._internal.core.models.runs import Job @@ -11,14 +11,25 @@ class JobConfiguration(CoreModel): volumes: List[Volume] -def fill_data(values: dict, filename_field: str = "filename", data_field: str = "data") -> dict: - if values.get(data_field) is not None: - return values - if (filename := values.get(filename_field)) is None: +M = TypeVar("M", bound=CoreModel) + + +def fill_data(model: M, filename_field: str = "filename", data_field: str = "data") -> M: + """ + Reads `data_field` from the file at `filename_field` unless it is already set. + + Call from an `after` validator: the fields must be validated already, or a malformed + `filename_field` reaches `open()` as-is and raises `TypeError` instead of a validation error. + """ + if getattr(model, data_field) is not None: + return model + filename = getattr(model, filename_field) + # An unset path field defaults to `""` in some configs and to `None` in others. + if not filename: raise ValueError(f"Either `{filename_field}` or `{data_field}` must be specified") try: with open(Path(filename).expanduser()) as f: - values[data_field] = f.read() + setattr(model, data_field, f.read()) except OSError: raise ValueError(f"No such file {filename}") - return values + return model diff --git a/src/dstack/_internal/core/backends/gcp/models.py b/src/dstack/_internal/core/backends/gcp/models.py index 47809329d..325cab629 100644 --- a/src/dstack/_internal/core/backends/gcp/models.py +++ b/src/dstack/_internal/core/backends/gcp/models.py @@ -1,6 +1,7 @@ from typing import Annotated, Dict, List, Literal, Optional, Union from pydantic import Field, RootModel, model_validator +from typing_extensions import Self from dstack._internal.core.backends.base.models import fill_data from dstack._internal.core.models.common import CoreModel @@ -121,10 +122,9 @@ class GCPServiceAccountFileCreds(CoreModel): ), ] = None - @model_validator(mode="before") - @classmethod - def fill_data(cls, values): - return fill_data(values) + @model_validator(mode="after") + def fill_data(self) -> Self: + return fill_data(self) AnyGCPFileCreds = Union[GCPServiceAccountFileCreds, GCPDefaultCreds] diff --git a/src/dstack/_internal/core/backends/kubernetes/models.py b/src/dstack/_internal/core/backends/kubernetes/models.py index c374bc8e6..c9df6ac88 100644 --- a/src/dstack/_internal/core/backends/kubernetes/models.py +++ b/src/dstack/_internal/core/backends/kubernetes/models.py @@ -1,6 +1,7 @@ from typing import Annotated, Literal, Optional, Union from pydantic import Field, model_validator +from typing_extensions import Self from dstack._internal.core.backends.base.models import fill_data from dstack._internal.core.models.common import CoreModel @@ -89,12 +90,9 @@ class KubeconfigFileConfig(CoreModel): ), ] = None - @model_validator(mode="before") - @classmethod - def fill_data(cls, values: dict) -> dict: - if values.get("filename") == "" and values.get("data") is None: - raise ValueError("filename or data must be specified") - return fill_data(values) + @model_validator(mode="after") + def fill_data(self) -> Self: + return fill_data(self) class KubernetesBackendFileConfigWithCreds(KubernetesBackendConfig): diff --git a/src/dstack/_internal/core/backends/nebius/models.py b/src/dstack/_internal/core/backends/nebius/models.py index 9adbc12a0..6c5e05dba 100644 --- a/src/dstack/_internal/core/backends/nebius/models.py +++ b/src/dstack/_internal/core/backends/nebius/models.py @@ -3,6 +3,7 @@ from typing import Annotated, Dict, Literal, Optional, Union from pydantic import Field, field_serializer, model_validator +from typing_extensions import Self from dstack._internal.core.backends.base.models import fill_data from dstack._internal.core.models.common import CoreModel @@ -76,10 +77,9 @@ class NebiusServiceAccountFileCreds(CoreModel): Optional[str], Field(description="The path to the service account credentials file") ] = None - @model_validator(mode="before") - @classmethod - def fill_data(cls, values): - if filename := values.get("filename"): + @model_validator(mode="after") + def fill_data(self) -> Self: + if filename := self.filename: try: with open(Path(filename).expanduser()) as f: data = json.load(f) @@ -89,18 +89,16 @@ def fill_data(cls, values): credentials = ServiceAccountCredentials.from_json(data) subject = credentials.subject_credentials - values["service_account_id"] = subject.sub - values["public_key_id"] = subject.kid - values["private_key_content"] = subject.private_key + self.service_account_id = subject.sub + self.public_key_id = subject.kid + self.private_key_content = subject.private_key except OSError: raise ValueError(f"No such file {filename}") except Exception as e: raise ValueError(f"Failed to parse credentials file {filename}: {e}") - return values + return self - return fill_data( - values, filename_field="private_key_file", data_field="private_key_content" - ) + return fill_data(self, filename_field="private_key_file", data_field="private_key_content") AnyNebiusCreds = NebiusServiceAccountCreds diff --git a/src/dstack/_internal/core/backends/slurm/models.py b/src/dstack/_internal/core/backends/slurm/models.py index f34e6b7e9..ccb472b91 100644 --- a/src/dstack/_internal/core/backends/slurm/models.py +++ b/src/dstack/_internal/core/backends/slurm/models.py @@ -1,6 +1,7 @@ from typing import Annotated, Literal, Optional, Union from pydantic import Field, model_validator +from typing_extensions import Self from dstack._internal.core.backends.base.models import fill_data from dstack._internal.core.models.common import CoreModel @@ -104,10 +105,9 @@ class SlurmPrivateKeyFileConfig(CoreModel): ), ] = None - @model_validator(mode="before") - @classmethod - def fill_data(cls, values: dict) -> dict: - return fill_data(values, filename_field="path", data_field="content") + @model_validator(mode="after") + def fill_data(self) -> Self: + return fill_data(self, filename_field="path", data_field="content") class SlurmClusterFileConfig(BaseSlurmClusterConfigWithCreds): diff --git a/src/tests/_internal/core/backends/base/test_models.py b/src/tests/_internal/core/backends/base/test_models.py new file mode 100644 index 000000000..f4959a95d --- /dev/null +++ b/src/tests/_internal/core/backends/base/test_models.py @@ -0,0 +1,83 @@ +from pathlib import Path +from typing import Any + +import pytest +from pydantic import ValidationError + +from dstack._internal.core.backends.gcp.models import GCPServiceAccountFileCreds +from dstack._internal.core.backends.kubernetes.models import KubeconfigFileConfig +from dstack._internal.core.backends.slurm.models import SlurmPrivateKeyFileConfig + + +class TestFillData: + def test_reads_data_from_file(self, tmp_path: Path): + creds_file = tmp_path / "creds.json" + creds_file.write_text("file-contents") + + creds = GCPServiceAccountFileCreds.model_validate({"filename": str(creds_file)}) + + assert creds.data == "file-contents" + + def test_keeps_data_and_does_not_read_the_file(self, tmp_path: Path): + creds = GCPServiceAccountFileCreds.model_validate( + {"filename": str(tmp_path / "missing"), "data": "explicit-contents"} + ) + + assert creds.data == "explicit-contents" + + def test_missing_file_rejected(self, tmp_path: Path): + with pytest.raises(ValidationError, match="No such file"): + GCPServiceAccountFileCreds.model_validate({"filename": str(tmp_path / "missing")}) + + @pytest.mark.parametrize( + "filename", + [ + pytest.param({"a": 1}, id="dict"), + pytest.param(["/tmp/x"], id="list"), + pytest.param(True, id="bool"), + ], + ) + def test_malformed_filename_reported_as_validation_error(self, filename: Any): + # A `before` validator would hand these straight to `open()`, raising `TypeError`. + with pytest.raises(ValidationError): + GCPServiceAccountFileCreds.model_validate({"filename": filename}) + + @pytest.mark.parametrize( + "obj", + [ + pytest.param("just-a-string", id="str"), + pytest.param(["filename", "/tmp/x"], id="list"), + pytest.param(12, id="int"), + ], + ) + def test_non_dict_input_reported_as_validation_error(self, obj: Any): + # A `before` validator would call `.get()` on these, raising `AttributeError`. + with pytest.raises(ValidationError): + GCPServiceAccountFileCreds.model_validate(obj) + + @pytest.mark.parametrize( + ["model", "message"], + [ + pytest.param( + GCPServiceAccountFileCreds, "Either `filename` or `data`", id="gcp-empty-filename" + ), + pytest.param( + KubeconfigFileConfig, "Either `filename` or `data`", id="kubernetes-empty-filename" + ), + ], + ) + def test_empty_filename_without_data_rejected(self, model: type, message: str): + with pytest.raises(ValidationError, match=message): + model.model_validate({"filename": ""}) + + def test_custom_field_names(self, tmp_path: Path): + key_file = tmp_path / "key" + key_file.write_text("private-key") + + config = SlurmPrivateKeyFileConfig.model_validate({"path": str(key_file)}) + + assert config.content == "private-key" + + def test_unset_path_without_content_rejected(self): + with pytest.raises(ValidationError, match="Either `path` or `content`"): + SlurmPrivateKeyFileConfig.model_validate({})