From aa64a2e8f59e37dfe84fb773462fedbb5c4712e4 Mon Sep 17 00:00:00 2001 From: AtomicGlance Date: Tue, 4 Aug 2026 21:58:02 +0330 Subject: [PATCH 1/6] 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/6] 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/6] 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 ( From 33c1e69f0262fede155694289de9d2866b6a0d49 Mon Sep 17 00:00:00 2001 From: AtomicGlance Date: Sat, 8 Aug 2026 19:39:07 +0330 Subject: [PATCH 4/6] Log validation summary before debug re-raise --- dandi/tests/fixtures.py | 9 ++--- dandi/tests/test_upload.py | 1 - dandi/upload.py | 67 +++++++++++++++++++------------------- 3 files changed, 36 insertions(+), 41 deletions(-) diff --git a/dandi/tests/fixtures.py b/dandi/tests/fixtures.py index 485cf36e6..4c4bb11a8 100644 --- a/dandi/tests/fixtures.py +++ b/dandi/tests/fixtures.py @@ -592,18 +592,13 @@ class SampleDandiset: def client(self) -> DandiAPIClient: return self.api.client - def upload( - self, - paths: list[str | Path] | None = None, - devel_debug: bool = True, - **kwargs: Any, - ) -> None: + def upload(self, paths: list[str | Path] | None = None, **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=devel_debug, + devel_debug=True, **{**self.upload_kwargs, **kwargs}, ) diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index 7f90768a6..ccf6371e6 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -220,7 +220,6 @@ 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 ( diff --git a/dandi/upload.py b/dandi/upload.py index 2af271d30..2f6fd186c 100644 --- a/dandi/upload.py +++ b/dandi/upload.py @@ -451,40 +451,41 @@ def upload_agg(*ignored: Any) -> str: style=pyout_style, columns=rec_fields, max_workers=jobs or 5 ) - with out: - for dfile in dandi_files: - while len(process_paths) >= 10: - lgr.log(2, "Sleep waiting for some paths to finish processing") - time.sleep(0.5) - - process_paths.add(str(dfile.filepath)) - - rec: dict[Any, Any] - if isinstance(dfile, DandisetMetadataFile): - rec = {"path": dandiset_metadata_file} - else: - assert isinstance(dfile, LocalAsset) - rec = {"path": dfile.path} - - try: - if devel_debug: - # DEBUG: do serially - for v in process_path(dfile): - print(str(v), flush=True) + try: + with out: + for dfile in dandi_files: + while len(process_paths) >= 10: + lgr.log(2, "Sleep waiting for some paths to finish processing") + time.sleep(0.5) + + process_paths.add(str(dfile.filepath)) + + rec: dict[Any, Any] + if isinstance(dfile, DandisetMetadataFile): + rec = {"path": dandiset_metadata_file} else: - rec[tuple(rec_fields[1:])] = process_path(dfile) - except ValueError as exc: - rec.update(error_file(exc)) - out(rec) - - if not validate_ok: - 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) + assert isinstance(dfile, LocalAsset) + rec = {"path": dfile.path} + + try: + if devel_debug: + # DEBUG: do serially + for v in process_path(dfile): + print(str(v), flush=True) + else: + rec[tuple(rec_fields[1:])] = process_path(dfile) + except ValueError as exc: + rec.update(error_file(exc)) + out(rec) + finally: + if not validate_ok: + 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 da745c7c9b7b538fadb293f097335bf75fd64782 Mon Sep 17 00:00:00 2001 From: AtomicGlance Date: Sat, 8 Aug 2026 19:43:05 +0330 Subject: [PATCH 5/6] Keep validation warning cleanup focused --- dandi/upload.py | 55 +++++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/dandi/upload.py b/dandi/upload.py index 2f6fd186c..6c13aca73 100644 --- a/dandi/upload.py +++ b/dandi/upload.py @@ -451,33 +451,7 @@ def upload_agg(*ignored: Any) -> str: style=pyout_style, columns=rec_fields, max_workers=jobs or 5 ) - try: - with out: - for dfile in dandi_files: - while len(process_paths) >= 10: - lgr.log(2, "Sleep waiting for some paths to finish processing") - time.sleep(0.5) - - process_paths.add(str(dfile.filepath)) - - rec: dict[Any, Any] - if isinstance(dfile, DandisetMetadataFile): - rec = {"path": dandiset_metadata_file} - else: - assert isinstance(dfile, LocalAsset) - rec = {"path": dfile.path} - - try: - if devel_debug: - # DEBUG: do serially - for v in process_path(dfile): - print(str(v), flush=True) - else: - rec[tuple(rec_fields[1:])] = process_path(dfile) - except ValueError as exc: - rec.update(error_file(exc)) - out(rec) - finally: + def report_validation_failure() -> None: if not validate_ok: msg = "One or more assets failed validation." if validation_log_path is not None: @@ -486,6 +460,33 @@ def upload_agg(*ignored: Any) -> str: " to review the saved results." ) lgr.warning(msg) + + with ExitStack() as warning_stack, out: + warning_stack.callback(report_validation_failure) + for dfile in dandi_files: + while len(process_paths) >= 10: + lgr.log(2, "Sleep waiting for some paths to finish processing") + time.sleep(0.5) + + process_paths.add(str(dfile.filepath)) + + rec: dict[Any, Any] + if isinstance(dfile, DandisetMetadataFile): + rec = {"path": dandiset_metadata_file} + else: + assert isinstance(dfile, LocalAsset) + rec = {"path": dfile.path} + + try: + if devel_debug: + # DEBUG: do serially + for v in process_path(dfile): + print(str(v), flush=True) + else: + rec[tuple(rec_fields[1:])] = process_path(dfile) + except ValueError as exc: + rec.update(error_file(exc)) + out(rec) if upload_err is not None: try: import etelemetry From f1f37edce34268a9a5379c971dfa3e696a3e437c Mon Sep 17 00:00:00 2001 From: AtomicGlance Date: Sun, 9 Aug 2026 17:21:10 +0330 Subject: [PATCH 6/6] Test validation failure with exception mapping disabled --- dandi/cli/tests/test_cmd_upload.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dandi/cli/tests/test_cmd_upload.py b/dandi/cli/tests/test_cmd_upload.py index dc6ff9c9d..937539dde 100644 --- a/dandi/cli/tests/test_cmd_upload.py +++ b/dandi/cli/tests/test_cmd_upload.py @@ -2,12 +2,14 @@ import pytest from pytest_mock import MockerFixture +from ..base import map_to_click_exceptions 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.object(map_to_click_exceptions, "_do_map", False) mocker.patch( "dandi.upload.upload", side_effect=UploadValidationError("failed validation"),