diff --git a/pyproject.toml b/pyproject.toml index d1f63d0c..d94af366 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -5,7 +5,7 @@ build-backend = "maturin" [project] name = "roar-cli" -version = "0.4.4" +version = "0.4.5" description = "Reproducibility and provenance tracker for ML training pipelines" authors = [ { name="TReqs Team", email="info@treqs.ai" } diff --git a/roar/core/validation.py b/roar/core/validation.py index 8f707dc2..cf1253ee 100644 --- a/roar/core/validation.py +++ b/roar/core/validation.py @@ -13,8 +13,11 @@ # Values that indicate missing/placeholder data and should never be sent to GLaaS FORBIDDEN_PLACEHOLDER_VALUES: frozenset[str | None] = frozenset({"unknown", "Unknown", "", None}) -# Valid source_type values for artifacts (None means local/lineage artifacts) -VALID_SOURCE_TYPES: frozenset[str | None] = frozenset({"s3", "gs", "https", None}) +# Valid source_type values for artifacts (None means local/lineage artifacts). +# Keep in step with `_VALID_REMOTE_SOURCE_TYPES` in application/publish/registration.py +# and with the receiver's enum; test_source_type_allowlists pins the two local sets +# together. +VALID_SOURCE_TYPES: frozenset[str | None] = frozenset({"s3", "gs", "https", "hf", None}) @dataclass @@ -146,7 +149,7 @@ def validate_artifact_registration( Args: hashes: List of hash entries [{algorithm, digest}, ...] size: File size in bytes - source_type: Type of artifact source ('s3', 'gs', 'https', or None for local) + source_type: Type of artifact source; see VALID_SOURCE_TYPES (None for local) session_hash: Session this artifact belongs to Returns: @@ -168,7 +171,8 @@ def validate_artifact_registration( # source_type must be one of the valid values (None is allowed for local artifacts) if source_type is not None and source_type not in VALID_SOURCE_TYPES: - errors.append(f"source_type must be 's3', 'gs', 'https', or None, got '{source_type}'") + allowed = ", ".join(sorted(repr(v) for v in VALID_SOURCE_TYPES if v is not None)) + errors.append(f"source_type must be one of {allowed}, or None, got '{source_type}'") if _is_placeholder(session_hash): errors.append("session_hash is required") diff --git a/tests/core/test_validation_source_types.py b/tests/core/test_validation_source_types.py new file mode 100644 index 00000000..e02aa2e9 --- /dev/null +++ b/tests/core/test_validation_source_types.py @@ -0,0 +1,68 @@ +"""The artifact source-type allowlists must agree with each other. + +roar keeps two of them: the validator's ``VALID_SOURCE_TYPES``, applied to every +primitive artifact registration, and ``_VALID_REMOTE_SOURCE_TYPES`` in the publish +path, applied when normalising a composite's source type. ``hf`` was added to the +publish-path set (and to the receiver) but not to the validator's, and the two sat +apart for months without anything noticing. + +Nothing noticed because the failure is quiet: a staged artifact that fails +validation is *skipped* and its message collected into a warnings list, so +``roar put … hf://`` still uploaded the file and still reported success — it simply +dropped the artifact's source from the registration. + +These tests pin the sets together so the next scheme cannot be half-added. +""" + +from __future__ import annotations + +from roar.application.publish.registration import _VALID_REMOTE_SOURCE_TYPES +from roar.core.validation import VALID_SOURCE_TYPES, validate_artifact_registration + + +def _artifact(source_type: str | None) -> dict: + return { + "hashes": [{"algorithm": "blake3", "digest": "d" * 64}], + "size": 1, + "source_type": source_type, + "session_hash": "a" * 64, + } + + +def test_the_two_local_allowlists_agree(): + """The validator must accept every scheme the publish path can emit. A scheme + the publish path stamps but the validator rejects is silently unregisterable.""" + validator_accepts = {v for v in VALID_SOURCE_TYPES if v is not None} + assert validator_accepts >= _VALID_REMOTE_SOURCE_TYPES + + +def test_every_remote_scheme_validates(): + for source_type in sorted(_VALID_REMOTE_SOURCE_TYPES): + assert validate_artifact_registration(**_artifact(source_type)), ( + f"{source_type!r} is emitted by the publish path but rejected by the validator" + ) + + +def test_hf_validates(): + """`roar put … hf://` stamps this on the artifact it publishes.""" + assert validate_artifact_registration(**_artifact("hf")) + + +def test_none_still_validates(): + """Local artifacts carry no source type.""" + assert validate_artifact_registration(**_artifact(None)) + + +def test_an_unknown_scheme_is_still_rejected(): + result = validate_artifact_registration(**_artifact("ftp")) + assert not result + assert any("source_type" in e for e in result.errors) + + +def test_the_rejection_message_lists_what_is_allowed(): + """The message used to hardcode its list and went stale the moment the set + changed; it is now derived from the set itself.""" + result = validate_artifact_registration(**_artifact("ftp")) + message = "; ".join(result.errors) + for allowed in (v for v in VALID_SOURCE_TYPES if v is not None): + assert repr(allowed) in message