Merge pull request #1080 from dandi/bids_upload_error

User notification if datasets are invalid.

dandi/tests/fixtures.py

@@ -447,6 +447,15 @@ def bids_dandiset(new_dandiset: SampleDandiset, bids_examples: str) -> SampleDan
     return new_dandiset
 
 
+@pytest.fixture()
+def bids_dandiset_invalid(new_dandiset: SampleDandiset, bids_examples: str) -> SampleDandiset:
+    copytree(
+        os.path.join(bids_examples, "invalid_pet001"),
+        str(new_dandiset.dspath) + "/",
+    )
+    return new_dandiset
+
+
 @pytest.fixture()
 def video_files(tmp_path):
     video_paths = []

dandi/tests/test_upload.py

@@ -181,9 +181,27 @@ def test_upload_sync_folder(
         text_dandiset.dandiset.get_asset_by_path("subdir2/banana.txt")
 
 
-def test_upload_bids(mocker: MockerFixture, bids_dandiset: SampleDandiset) -> None:
+def test_upload_bids_invalid(
+    mocker: MockerFixture, bids_dandiset_invalid: SampleDandiset
+) -> None:
     iter_upload_spy = mocker.spy(LocalFileAsset, "iter_upload")
-    bids_dandiset.upload(existing="forced")
+    # Does it fail when it should fail?
+    with pytest.raises(RuntimeError):
+        bids_dandiset_invalid.upload(existing="forced")
+    iter_upload_spy.assert_not_called()
+    # Does validation ignoring work?
+    bids_dandiset_invalid.upload(existing="forced", validation="ignore")
+    iter_upload_spy.assert_called()
+    # Check existence of assets:
+    dandiset = bids_dandiset_invalid.dandiset
+    dandiset.get_asset_by_path("dataset_description.json")
+
+
+def test_upload_bids_validation_ignore(
+    mocker: MockerFixture, bids_dandiset: SampleDandiset
+) -> None:
+    iter_upload_spy = mocker.spy(LocalFileAsset, "iter_upload")
+    bids_dandiset.upload(existing="forced", validation="ignore")
     # Check whether upload was run
     iter_upload_spy.assert_called()
     # Check existence of assets:
@@ -196,11 +214,9 @@ def test_upload_bids(mocker: MockerFixture, bids_dandiset: SampleDandiset) -> No
     dandiset.get_asset_by_path("sub-Sub1/anat/sub-Sub1_T1w.nii.gz")
 
 
-def test_upload_bids_validation_ignore(
-    mocker: MockerFixture, bids_dandiset: SampleDandiset
-) -> None:
+def test_upload_bids(mocker: MockerFixture, bids_dandiset: SampleDandiset) -> None:
     iter_upload_spy = mocker.spy(LocalFileAsset, "iter_upload")
-    bids_dandiset.upload(existing="forced", validation="ignore")
+    bids_dandiset.upload(existing="forced")
     # Check whether upload was run
     iter_upload_spy.assert_called()
     # Check existence of assets:

dandi/upload.py

@@ -450,16 +450,25 @@ def _bids_discover_and_validate(
         else:
             bids_datasets_to_validate = bids_datasets
         bids_datasets_to_validate.sort()
-        validated_datasets = []
+        valid_datasets: List[Path] = []
+        invalid_datasets: List[Path] = []
         for bd in bids_datasets_to_validate:
             validator_result = validate_bids(bd)
             valid = is_valid(
                 validator_result,
                 allow_missing_files=validation == "ignore",
                 allow_invalid_filenames=validation == "ignore",
             )
-            if valid:
-                validated_datasets.append(bd)
-        return validated_datasets
+            (valid_datasets if valid else invalid_datasets).append(bd)
+        if invalid_datasets:
+            raise RuntimeError(
+                f"Found {pluralize(len(invalid_datasets), 'BIDS dataset')}, which did not "
+                "pass validation:\n * "
+                + "\n * ".join([str(i) for i in invalid_datasets])
+                + "\nTo resolve "
+                "this, perform the required changes or set the validation parameter to "
+                '"skip" or "ignore".'
+            )
+        return valid_datasets
     else:
         return bids_datasets