From aa64a2e8f59e37dfe84fb773462fedbb5c4712e4 Mon Sep 17 00:00:00 2001 From: AtomicGlance Date: Tue, 4 Aug 2026 21:58:02 +0330 Subject: [PATCH 1/3] fix: point failed uploads to validation report --- dandi/cli/cmd_upload.py | 30 +++++++++++++++++------------- dandi/cli/tests/test_cmd_upload.py | 20 ++++++++++++++++++++ dandi/exceptions.py | 6 ++++++ dandi/tests/test_upload.py | 16 ++++++++++++++-- dandi/upload.py | 16 ++++++++++------ 5 files changed, 67 insertions(+), 21 deletions(-) create mode 100644 dandi/cli/tests/test_cmd_upload.py diff --git a/dandi/cli/cmd_upload.py b/dandi/cli/cmd_upload.py index 9b5f1760c..772a05a30 100644 --- a/dandi/cli/cmd_upload.py +++ b/dandi/cli/cmd_upload.py @@ -11,6 +11,7 @@ map_to_click_exceptions, ) from ..consts import SyncMode +from ..exceptions import UploadValidationError from ..upload import UploadExisting, UploadValidation @@ -119,16 +120,19 @@ def upload( validation_companion_path(ctx.obj.logfile) if ctx.obj is not None else None ) - upload_( - paths, - existing=existing, - validation=validation, - dandi_instance=dandi_instance, - allow_any_path=allow_any_path, - upload_dandiset_metadata=upload_dandiset_metadata, - devel_debug=devel_debug, - jobs=jobs, - jobs_per_file=jobs_per_file, - sync=SyncMode(sync) if sync is not None else None, - validation_log_path=companion, - ) + try: + upload_( + paths, + existing=existing, + validation=validation, + dandi_instance=dandi_instance, + allow_any_path=allow_any_path, + upload_dandiset_metadata=upload_dandiset_metadata, + devel_debug=devel_debug, + jobs=jobs, + jobs_per_file=jobs_per_file, + sync=SyncMode(sync) if sync is not None else None, + validation_log_path=companion, + ) + except UploadValidationError as exc: + raise click.ClickException(str(exc)) diff --git a/dandi/cli/tests/test_cmd_upload.py b/dandi/cli/tests/test_cmd_upload.py new file mode 100644 index 000000000..dc6ff9c9d --- /dev/null +++ b/dandi/cli/tests/test_cmd_upload.py @@ -0,0 +1,20 @@ +from click.testing import CliRunner +import pytest +from pytest_mock import MockerFixture + +from ..cmd_upload import upload +from ...exceptions import UploadValidationError + + +@pytest.mark.ai_generated +def test_upload_validation_error_has_no_traceback(mocker: MockerFixture) -> None: + mocker.patch( + "dandi.upload.upload", + side_effect=UploadValidationError("failed validation"), + ) + + result = CliRunner().invoke(upload) + + assert result.exit_code == 1 + assert result.output == "Error: failed validation\n" + assert "Traceback" not in result.output diff --git a/dandi/exceptions.py b/dandi/exceptions.py index fc8639dff..f323d2a42 100644 --- a/dandi/exceptions.py +++ b/dandi/exceptions.py @@ -91,3 +91,9 @@ class HTTP404Error(requests.HTTPError): class UploadError(Exception): pass + + +class UploadValidationError(UploadError): + """An upload could not proceed because an asset failed validation.""" + + pass diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index cda476474..ccf6371e6 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -207,13 +207,25 @@ def test_upload_sync_do(mocker: MockerFixture, text_dandiset: SampleDandiset) -> text_dandiset.dandiset.get_asset_by_path("file.txt") +@pytest.mark.ai_generated def test_upload_bids_invalid( - mocker: MockerFixture, bids_dandiset_invalid: SampleDandiset + caplog: pytest.LogCaptureFixture, + mocker: MockerFixture, + bids_dandiset_invalid: SampleDandiset, + tmp_path: Path, ) -> None: iter_upload_spy = mocker.spy(LocalFileAsset, "iter_upload") + validation_log = tmp_path / "upload_validation.jsonl" with pytest.raises(UploadError): - bids_dandiset_invalid.upload(existing=UploadExisting.FORCE) + bids_dandiset_invalid.upload( + existing=UploadExisting.FORCE, + validation_log_path=validation_log, + ) iter_upload_spy.assert_not_called() + assert ( + f"Use `dandi validate --load {validation_log}` to review the saved results." + in caplog.text + ) # Does validation ignoring work? bids_dandiset_invalid.upload( existing=UploadExisting.FORCE, validation=UploadValidation.IGNORE diff --git a/dandi/upload.py b/dandi/upload.py index e9d5b2402..fe0b1310e 100644 --- a/dandi/upload.py +++ b/dandi/upload.py @@ -38,7 +38,7 @@ ) from .dandiapi import DandiAPIClient, RemoteAsset from .dandiset import Dandiset -from .exceptions import NotFoundError, UploadError +from .exceptions import NotFoundError, UploadError, UploadValidationError from .files import ( DandiFile, DandisetMetadataFile, @@ -318,7 +318,7 @@ def process_path(dfile: DandiFile) -> Iterator[dict]: for i, e in enumerate(validation_errors, start=1): lgr.warning(" Error %d: %s", i, e) validate_ok = False - raise UploadError("failed validation") + raise UploadValidationError("failed validation") else: yield {"status": "validated"} else: @@ -478,10 +478,14 @@ def upload_agg(*ignored: Any) -> str: out(rec) if not validate_ok: - lgr.warning( - "One or more assets failed validation. Consult the logfile for" - " details." - ) + if validation_log_path is None: + lgr.warning("One or more assets failed validation.") + else: + lgr.warning( + "One or more assets failed validation. Use" + " `dandi validate --load %s` to review the saved results.", + validation_log_path, + ) if upload_err is not None: try: import etelemetry From 5fa28eff8f6f302dd719a308e9693b24ba744763 Mon Sep 17 00:00:00 2001 From: AtomicGlance Date: Fri, 7 Aug 2026 01:19:59 +0330 Subject: [PATCH 2/3] Simplify validation failure warning --- dandi/upload.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/dandi/upload.py b/dandi/upload.py index fe0b1310e..2af271d30 100644 --- a/dandi/upload.py +++ b/dandi/upload.py @@ -478,14 +478,13 @@ def upload_agg(*ignored: Any) -> str: out(rec) if not validate_ok: - if validation_log_path is None: - lgr.warning("One or more assets failed validation.") - else: - lgr.warning( - "One or more assets failed validation. Use" - " `dandi validate --load %s` to review the saved results.", - validation_log_path, + msg = "One or more assets failed validation." + if validation_log_path is not None: + msg += ( + f" Use `dandi validate --load {validation_log_path}`" + " to review the saved results." ) + lgr.warning(msg) if upload_err is not None: try: import etelemetry From 3d5895cb47c635e9c8366cb7ede67cf10bc15293 Mon Sep 17 00:00:00 2001 From: AtomicGlance Date: Sat, 8 Aug 2026 09:27:17 +0330 Subject: [PATCH 3/3] Test validation warning in normal upload mode --- dandi/tests/fixtures.py | 9 +++++++-- dandi/tests/test_upload.py | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/dandi/tests/fixtures.py b/dandi/tests/fixtures.py index 4c4bb11a8..485cf36e6 100644 --- a/dandi/tests/fixtures.py +++ b/dandi/tests/fixtures.py @@ -592,13 +592,18 @@ class SampleDandiset: def client(self) -> DandiAPIClient: return self.api.client - def upload(self, paths: list[str | Path] | None = None, **kwargs: Any) -> None: + def upload( + self, + paths: list[str | Path] | None = None, + devel_debug: bool = True, + **kwargs: Any, + ) -> None: with pytest.MonkeyPatch().context() as m: self.api.monkeypatch_set_api_key_env(m) upload( paths=paths or [self.dspath], dandi_instance=self.api.instance_id, - devel_debug=True, + devel_debug=devel_debug, **{**self.upload_kwargs, **kwargs}, ) diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index ccf6371e6..7f90768a6 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -220,6 +220,7 @@ def test_upload_bids_invalid( bids_dandiset_invalid.upload( existing=UploadExisting.FORCE, validation_log_path=validation_log, + devel_debug=False, ) iter_upload_spy.assert_not_called() assert (