From 878fc81f244c8de2e872d8ecf9983e30f0c83069 Mon Sep 17 00:00:00 2001 From: Horea Christian Date: Tue, 19 Jul 2022 14:17:31 -0400 Subject: [PATCH 01/12] User notification if datasets are invalid. --- dandi/upload.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/dandi/upload.py b/dandi/upload.py index f79369067..1d0d45714 100644 --- a/dandi/upload.py +++ b/dandi/upload.py @@ -453,6 +453,19 @@ def _bids_discover_and_validate( ) if valid: validated_datasets.append(bd) + invalid_datasets = [] + for i in bids_datasets_to_validate: + if i not in validated_datasets: + invalid_datasets.append(str(i)) + if invalid_datasets: + raise RuntimeError( + f"Found {pluralize(len(invalid_datasets), 'BIDS dataset')}, which did not " + "pass validation:\n * " + + "\n * ".join(invalid_datasets) + + "\nTo resolve " + "this, perform the required changes or set the validation parameter to " + '"skip" or "ignore".' + ) return validated_datasets else: return bids_datasets From a3c6b68250d9de6370e7612fc8195a1c7a7ce97f Mon Sep 17 00:00:00 2001 From: Horea Christian Date: Thu, 21 Jul 2022 15:31:42 -0400 Subject: [PATCH 02/12] streamlined list population --- dandi/upload.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/dandi/upload.py b/dandi/upload.py index 1d0d45714..9dc11a808 100644 --- a/dandi/upload.py +++ b/dandi/upload.py @@ -443,7 +443,8 @@ def _bids_discover_and_validate( else: bids_datasets_to_validate = bids_datasets bids_datasets_to_validate.sort() - validated_datasets = [] + valid_datasets = [] + invalid_datasets = [] for bd in bids_datasets_to_validate: validator_result = validate_bids(bd) valid = is_valid( @@ -451,12 +452,7 @@ def _bids_discover_and_validate( allow_missing_files=validation == "ignore", allow_invalid_filenames=validation == "ignore", ) - if valid: - validated_datasets.append(bd) - invalid_datasets = [] - for i in bids_datasets_to_validate: - if i not in validated_datasets: - invalid_datasets.append(str(i)) + (valid_datasets if valid else invalid_datasets).append(str(bd)) if invalid_datasets: raise RuntimeError( f"Found {pluralize(len(invalid_datasets), 'BIDS dataset')}, which did not " @@ -466,6 +462,6 @@ def _bids_discover_and_validate( "this, perform the required changes or set the validation parameter to " '"skip" or "ignore".' ) - return validated_datasets + return valid_datasets else: return bids_datasets From 6c4e18e0bff682aa023601989d4d75fb19dfac79 Mon Sep 17 00:00:00 2001 From: Horea Christian Date: Thu, 21 Jul 2022 15:37:41 -0400 Subject: [PATCH 03/12] Typing fix --- dandi/upload.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dandi/upload.py b/dandi/upload.py index 9dc11a808..63cc9d2ee 100644 --- a/dandi/upload.py +++ b/dandi/upload.py @@ -443,7 +443,7 @@ def _bids_discover_and_validate( else: bids_datasets_to_validate = bids_datasets bids_datasets_to_validate.sort() - valid_datasets = [] + valid_datasets: List[str] = [] invalid_datasets = [] for bd in bids_datasets_to_validate: validator_result = validate_bids(bd) From b908987b5c1cd6a03d7bea8b10581c5e04197982 Mon Sep 17 00:00:00 2001 From: Horea Christian Date: Thu, 28 Jul 2022 15:09:48 -0400 Subject: [PATCH 04/12] Typing fix --- dandi/upload.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dandi/upload.py b/dandi/upload.py index 63cc9d2ee..9096d9244 100644 --- a/dandi/upload.py +++ b/dandi/upload.py @@ -443,8 +443,8 @@ def _bids_discover_and_validate( else: bids_datasets_to_validate = bids_datasets bids_datasets_to_validate.sort() - valid_datasets: List[str] = [] - invalid_datasets = [] + valid_datasets: List[Path] = [] + invalid_datasets: List[Path] = [] for bd in bids_datasets_to_validate: validator_result = validate_bids(bd) valid = is_valid( From 10d20f5a77e8735e178efeb4a15dd9c43fd31fd2 Mon Sep 17 00:00:00 2001 From: Horea Christian Date: Thu, 28 Jul 2022 15:49:05 -0400 Subject: [PATCH 05/12] Adapting code to type --- dandi/upload.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dandi/upload.py b/dandi/upload.py index 9096d9244..f0e718760 100644 --- a/dandi/upload.py +++ b/dandi/upload.py @@ -452,12 +452,12 @@ def _bids_discover_and_validate( allow_missing_files=validation == "ignore", allow_invalid_filenames=validation == "ignore", ) - (valid_datasets if valid else invalid_datasets).append(str(bd)) + (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(invalid_datasets) + + "\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".' From 420e2860352cbdc8c9da118a546aab4222129075 Mon Sep 17 00:00:00 2001 From: Horea Christian Date: Thu, 28 Jul 2022 16:50:42 -0400 Subject: [PATCH 06/12] Error testing --- dandi/tests/fixtures.py | 9 +++++++++ dandi/tests/test_upload.py | 21 +++++++++++++++------ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/dandi/tests/fixtures.py b/dandi/tests/fixtures.py index 7a8b21998..3be463b18 100644 --- a/dandi/tests/fixtures.py +++ b/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 = [] diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index 27ee93c08..fe8d04fc5 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -181,9 +181,20 @@ 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") + bids_dandiset_invalid.upload(existing="forced") + # Check whether upload failed + iter_upload_spy.assert_not_called() + + +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 +207,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: From c5869e4924cde115d756b4f0a59facb670d77a40 Mon Sep 17 00:00:00 2001 From: Horea Christian Date: Fri, 29 Jul 2022 05:22:38 -0400 Subject: [PATCH 07/12] Catching expected exception --- dandi/tests/test_upload.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index fe8d04fc5..6ec93dc06 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -185,8 +185,8 @@ def test_upload_bids_invalid( mocker: MockerFixture, bids_dandiset_invalid: SampleDandiset ) -> None: iter_upload_spy = mocker.spy(LocalFileAsset, "iter_upload") - bids_dandiset_invalid.upload(existing="forced") - # Check whether upload failed + with pytest.raises(RuntimeError): + bids_dandiset_invalid.upload(existing="forced") iter_upload_spy.assert_not_called() From b6756472723316168889aed97fcf7da98d4e6315 Mon Sep 17 00:00:00 2001 From: Horea Christian Date: Fri, 29 Jul 2022 13:48:55 -0400 Subject: [PATCH 08/12] Testing ignore case --- dandi/tests/test_upload.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index 6ec93dc06..d64c44d56 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -188,6 +188,11 @@ def test_upload_bids_invalid( 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_once() + # Are files uploaded? + dandiset.get_asset_by_path("dataset_description.json") def test_upload_bids_validation_ignore( From 52a16a91aa1d80bb4786c792d526126556eb1953 Mon Sep 17 00:00:00 2001 From: Horea Christian Date: Fri, 29 Jul 2022 13:50:53 -0400 Subject: [PATCH 09/12] Commenting test cases --- dandi/tests/test_upload.py | 1 + 1 file changed, 1 insertion(+) diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index d64c44d56..4f2fe4638 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -185,6 +185,7 @@ def test_upload_bids_invalid( mocker: MockerFixture, bids_dandiset_invalid: SampleDandiset ) -> None: iter_upload_spy = mocker.spy(LocalFileAsset, "iter_upload") + # Does it fail when it should fail? with pytest.raises(RuntimeError): bids_dandiset_invalid.upload(existing="forced") iter_upload_spy.assert_not_called() From 90900acce725cac5b1a02fc4f50c3b7ccc089273 Mon Sep 17 00:00:00 2001 From: Horea Christian Date: Fri, 29 Jul 2022 13:52:17 -0400 Subject: [PATCH 10/12] Corrected variable name --- dandi/tests/test_upload.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index 4f2fe4638..7c6f3bfdc 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -193,7 +193,7 @@ def test_upload_bids_invalid( bids_dandiset_invalid.upload(existing="forced", validation="ignore") iter_upload_spy.assert_called_once() # Are files uploaded? - dandiset.get_asset_by_path("dataset_description.json") + bids_dandiset_invalid.get_asset_by_path("dataset_description.json") def test_upload_bids_validation_ignore( From c788df2327aa7d4d66f89b77a49d71cdefcab5f2 Mon Sep 17 00:00:00 2001 From: Horea Christian Date: Fri, 29 Jul 2022 14:30:00 -0400 Subject: [PATCH 11/12] Checking asset --- dandi/tests/test_upload.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index 7c6f3bfdc..0f67032a8 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -192,8 +192,10 @@ def test_upload_bids_invalid( # Does validation ignoring work? bids_dandiset_invalid.upload(existing="forced", validation="ignore") iter_upload_spy.assert_called_once() - # Are files uploaded? - bids_dandiset_invalid.get_asset_by_path("dataset_description.json") + # Check existence of assets: + dandiset = bids_dandiset_invalid.dandiset + # Check file existence: + dandiset.get_asset_by_path("dataset_description.json") def test_upload_bids_validation_ignore( From 6bf1847f5d51add97ed59bd2312650219f8a8ed7 Mon Sep 17 00:00:00 2001 From: Horea Christian Date: Fri, 29 Jul 2022 14:58:02 -0400 Subject: [PATCH 12/12] Checking upload correctly --- dandi/tests/test_upload.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index 0f67032a8..39cd611bb 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -191,10 +191,9 @@ def test_upload_bids_invalid( iter_upload_spy.assert_not_called() # Does validation ignoring work? bids_dandiset_invalid.upload(existing="forced", validation="ignore") - iter_upload_spy.assert_called_once() + iter_upload_spy.assert_called() # Check existence of assets: dandiset = bids_dandiset_invalid.dandiset - # Check file existence: dandiset.get_asset_by_path("dataset_description.json")