From 957da049abd6589ce7810afc5ac9f5da2c247880 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 12 Nov 2025 21:02:22 +0000 Subject: [PATCH 1/6] Add HINT validation result for dandiset.yaml in BIDS datasets When BIDS validator encounters dandiset.yaml (not part of BIDS spec), now: - Keeps the existing ERROR about the file not matching BIDS patterns - Adds a new HINT level validation result suggesting to create/update .bidsignore Includes test to verify both ERROR and HINT are generated Co-authored-by: yarikoptic <39889+yarikoptic@users.noreply.github.com> --- dandi/tests/test_validate.py | 20 ++++++++++++++++++++ dandi/validate.py | 26 ++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/dandi/tests/test_validate.py b/dandi/tests/test_validate.py index 5b240eff8..45be0abac 100644 --- a/dandi/tests/test_validate.py +++ b/dandi/tests/test_validate.py @@ -120,6 +120,26 @@ def mock_bids_validate(*args: Any, **kwargs: Any) -> list[ValidationResult]: f"BIDS specification." ) + # Check that there is also a HINT about .bidsignore + validation_hints = [ + r + for r in validation_results + if r.severity is not None and r.severity == Severity.HINT + ] + + # Assert that there is at least one hint + assert len(validation_hints) >= 1 + + # Find the hint about .bidsignore for dandiset.yaml + bidsignore_hint = next( + (h for h in validation_hints if h.id == "DANDI.BIDSIGNORE_DANDISET_YAML"), + None, + ) + assert bidsignore_hint is not None + assert bidsignore_hint.message is not None + assert ".bidsignore" in bidsignore_hint.message + assert dandiset_metadata_file in bidsignore_hint.message + def test_validate_bids_onefile(bids_error_examples: Path, tmp_path: Path) -> None: """ diff --git a/dandi/validate.py b/dandi/validate.py index e67dfb383..7b9e7581b 100644 --- a/dandi/validate.py +++ b/dandi/validate.py @@ -188,12 +188,14 @@ def validate( if r_id not in df_result_ids: # If the error is about the dandiset metadata file, modify # the message in the validation to give the context of DANDI - if ( + # and add a HINT to suggest adding to .bidsignore + is_dandiset_yaml_error = ( r.path is not None and r.dataset_path is not None and r.path.relative_to(r.dataset_path).as_posix() == dandiset_metadata_file - ): + ) + if is_dandiset_yaml_error: r.message = ( f"The dandiset metadata file, `{dandiset_metadata_file}`, " f"is not a part of BIDS specification. Please include a " @@ -205,3 +207,23 @@ def validate( df_results.append(r) df_result_ids.add(r_id) yield r + + # After yielding the error for dandiset.yaml, also yield a HINT + if is_dandiset_yaml_error: + hint = ValidationResult( + id="DANDI.BIDSIGNORE_DANDISET_YAML", + origin=ORIGIN_VALIDATION_DANDI_LAYOUT, + severity=Severity.HINT, + scope=Scope.DATASET, + path=r.dataset_path, + dataset_path=r.dataset_path, + dandiset_path=dandiset_path, + message=( + f"Consider creating or updating a `.bidsignore` file " + f"in the root of your BIDS dataset to ignore " + f"`{dandiset_metadata_file}`. " + f"Add the following line to `.bidsignore`:\n" + f"{dandiset_metadata_file}" + ), + ) + yield hint From 6faecd2ab0592800d811c616a6879d771d29ff0b Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Wed, 7 Jan 2026 12:18:33 -0500 Subject: [PATCH 2/6] RF: do not adjust existing message, add a HINT one and adjust record per Isaac review --- dandi/validate.py | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/dandi/validate.py b/dandi/validate.py index 7b9e7581b..0014f8bda 100644 --- a/dandi/validate.py +++ b/dandi/validate.py @@ -186,8 +186,12 @@ def validate( ): r_id = id(r) if r_id not in df_result_ids: - # If the error is about the dandiset metadata file, modify - # the message in the validation to give the context of DANDI + df_results.append(r) + df_result_ids.add(r_id) + yield r + + # If the error is about the dandiset metadata file, add a HINT + # message in the validation to give the context of DANDI # and add a HINT to suggest adding to .bidsignore is_dandiset_yaml_error = ( r.path is not None @@ -195,29 +199,16 @@ def validate( and r.path.relative_to(r.dataset_path).as_posix() == dandiset_metadata_file ) - if is_dandiset_yaml_error: - r.message = ( - f"The dandiset metadata file, `{dandiset_metadata_file}`, " - f"is not a part of BIDS specification. Please include a " - f"`.bidsignore` file with specification to ignore the " - f"metadata file in your dataset. For more details, see " - f"https://github.com/bids-standard/bids-specification/" - f"issues/131#issuecomment-461060166." - ) - df_results.append(r) - df_result_ids.add(r_id) - yield r - - # After yielding the error for dandiset.yaml, also yield a HINT if is_dandiset_yaml_error: hint = ValidationResult( id="DANDI.BIDSIGNORE_DANDISET_YAML", origin=ORIGIN_VALIDATION_DANDI_LAYOUT, - severity=Severity.HINT, scope=Scope.DATASET, - path=r.dataset_path, + origin_result=r, + severity=Severity.HINT, + dandiset_path=r.dandiset_path, dataset_path=r.dataset_path, - dandiset_path=dandiset_path, + path=r.path, message=( f"Consider creating or updating a `.bidsignore` file " f"in the root of your BIDS dataset to ignore " From bbe7870b536c5bc8da7a94408f52f140e9e0d4cd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 8 Jan 2026 00:42:52 +0000 Subject: [PATCH 3/6] Move HINT generation to BIDS validator harmonization - Moved dandiset.yaml HINT logic from validate() to _harmonize() in bids_validator_deno - This ensures HINT appears for both 'dandi validate' and 'dandi upload' commands - Removed modification of original BIDS error message (preserving original error) - Updated test to not expect modified error message - All 101 validation tests passing Co-authored-by: yarikoptic <39889+yarikoptic@users.noreply.github.com> --- dandi/bids_validator_deno/_validator.py | 47 +++++++++++++++++++------ dandi/tests/test_validate.py | 7 ++-- dandi/validate.py | 29 --------------- 3 files changed, 40 insertions(+), 43 deletions(-) diff --git a/dandi/bids_validator_deno/_validator.py b/dandi/bids_validator_deno/_validator.py index d97f953db..7e28bb7cc 100644 --- a/dandi/bids_validator_deno/_validator.py +++ b/dandi/bids_validator_deno/_validator.py @@ -12,8 +12,10 @@ from packaging.version import parse as parse_ver_str from pydantic import DirectoryPath, validate_call +from dandi.consts import dandiset_metadata_file from dandi.utils import find_parent_directory_containing from dandi.validate_types import ( + ORIGIN_VALIDATION_DANDI_LAYOUT, Origin, OriginType, Scope, @@ -361,20 +363,45 @@ def _harmonize( # The absolute path to the file or directory that the issue is related to issue_path = _get_path(issue, ds_path) - results.append( - ValidationResult( - id=f"BIDS.{issue.code}", - origin=origin, - scope=_get_scope(issue_path), - # Store only the issue, not entire bv_result with more context - origin_result=issue, - severity=_SEVERITY_MAP[severity] if severity else None, + validation_result = ValidationResult( + id=f"BIDS.{issue.code}", + origin=origin, + scope=_get_scope(issue_path), + # Store only the issue, not entire bv_result with more context + origin_result=issue, + severity=_SEVERITY_MAP[severity] if severity else None, + dandiset_path=dandiset_path, + dataset_path=ds_path, + message=_get_msg(issue, code_messages), + path=issue_path, + ) + results.append(validation_result) + + # If the error is about the dandiset metadata file, add a HINT + # suggesting to add it to .bidsignore + is_dandiset_yaml_error = ( + issue_path is not None + and issue_path.relative_to(ds_path).as_posix() == dandiset_metadata_file + ) + if is_dandiset_yaml_error: + hint = ValidationResult( + id="DANDI.BIDSIGNORE_DANDISET_YAML", + origin=ORIGIN_VALIDATION_DANDI_LAYOUT, + scope=Scope.DATASET, + origin_result=validation_result, + severity=Severity.HINT, dandiset_path=dandiset_path, dataset_path=ds_path, - message=_get_msg(issue, code_messages), path=issue_path, + message=( + f"Consider creating or updating a `.bidsignore` file " + f"in the root of your BIDS dataset to ignore " + f"`{dandiset_metadata_file}`. " + f"Add the following line to `.bidsignore`:\n" + f"{dandiset_metadata_file}" + ), ) - ) + results.append(hint) return results diff --git a/dandi/tests/test_validate.py b/dandi/tests/test_validate.py index 45be0abac..703de8730 100644 --- a/dandi/tests/test_validate.py +++ b/dandi/tests/test_validate.py @@ -114,11 +114,10 @@ def mock_bids_validate(*args: Any, **kwargs: Any) -> list[ValidationResult]: assert err.dataset_path is not None assert err.path.relative_to(err.dataset_path).as_posix() == dandiset_metadata_file + # The error message should be the original BIDS error, not modified assert err.message is not None - assert err.message.startswith( - f"The dandiset metadata file, `{dandiset_metadata_file}`, is not a part of " - f"BIDS specification." - ) + # We just check that it's about dandiset.yaml, not checking exact message + # since it comes from BIDS validator # Check that there is also a HINT about .bidsignore validation_hints = [ diff --git a/dandi/validate.py b/dandi/validate.py index 0014f8bda..3b4dae26f 100644 --- a/dandi/validate.py +++ b/dandi/validate.py @@ -189,32 +189,3 @@ def validate( df_results.append(r) df_result_ids.add(r_id) yield r - - # If the error is about the dandiset metadata file, add a HINT - # message in the validation to give the context of DANDI - # and add a HINT to suggest adding to .bidsignore - is_dandiset_yaml_error = ( - r.path is not None - and r.dataset_path is not None - and r.path.relative_to(r.dataset_path).as_posix() - == dandiset_metadata_file - ) - if is_dandiset_yaml_error: - hint = ValidationResult( - id="DANDI.BIDSIGNORE_DANDISET_YAML", - origin=ORIGIN_VALIDATION_DANDI_LAYOUT, - scope=Scope.DATASET, - origin_result=r, - severity=Severity.HINT, - dandiset_path=r.dandiset_path, - dataset_path=r.dataset_path, - path=r.path, - message=( - f"Consider creating or updating a `.bidsignore` file " - f"in the root of your BIDS dataset to ignore " - f"`{dandiset_metadata_file}`. " - f"Add the following line to `.bidsignore`:\n" - f"{dandiset_metadata_file}" - ), - ) - yield hint From ba523e61e923a3de458aec71d2ad0993624c3227 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 14 Jan 2026 19:54:10 +0000 Subject: [PATCH 4/6] Move HINT generation from bids_validator_deno to bids.py - Removed HINT generation logic from _harmonize() in bids_validator_deno/_validator.py - Added HINT generation in _validate() in dandi/files/bids.py after line 115 - This keeps bids_validator_deno true to what BIDS validator deno returns - HINT is now generated at the BIDS file interface level - All 101 validation tests passing Co-authored-by: yarikoptic <39889+yarikoptic@users.noreply.github.com> --- dandi/bids_validator_deno/_validator.py | 47 ++++++------------------- dandi/files/bids.py | 42 ++++++++++++++++++++-- 2 files changed, 50 insertions(+), 39 deletions(-) diff --git a/dandi/bids_validator_deno/_validator.py b/dandi/bids_validator_deno/_validator.py index 7e28bb7cc..d97f953db 100644 --- a/dandi/bids_validator_deno/_validator.py +++ b/dandi/bids_validator_deno/_validator.py @@ -12,10 +12,8 @@ from packaging.version import parse as parse_ver_str from pydantic import DirectoryPath, validate_call -from dandi.consts import dandiset_metadata_file from dandi.utils import find_parent_directory_containing from dandi.validate_types import ( - ORIGIN_VALIDATION_DANDI_LAYOUT, Origin, OriginType, Scope, @@ -363,45 +361,20 @@ def _harmonize( # The absolute path to the file or directory that the issue is related to issue_path = _get_path(issue, ds_path) - validation_result = ValidationResult( - id=f"BIDS.{issue.code}", - origin=origin, - scope=_get_scope(issue_path), - # Store only the issue, not entire bv_result with more context - origin_result=issue, - severity=_SEVERITY_MAP[severity] if severity else None, - dandiset_path=dandiset_path, - dataset_path=ds_path, - message=_get_msg(issue, code_messages), - path=issue_path, - ) - results.append(validation_result) - - # If the error is about the dandiset metadata file, add a HINT - # suggesting to add it to .bidsignore - is_dandiset_yaml_error = ( - issue_path is not None - and issue_path.relative_to(ds_path).as_posix() == dandiset_metadata_file - ) - if is_dandiset_yaml_error: - hint = ValidationResult( - id="DANDI.BIDSIGNORE_DANDISET_YAML", - origin=ORIGIN_VALIDATION_DANDI_LAYOUT, - scope=Scope.DATASET, - origin_result=validation_result, - severity=Severity.HINT, + results.append( + ValidationResult( + id=f"BIDS.{issue.code}", + origin=origin, + scope=_get_scope(issue_path), + # Store only the issue, not entire bv_result with more context + origin_result=issue, + severity=_SEVERITY_MAP[severity] if severity else None, dandiset_path=dandiset_path, dataset_path=ds_path, + message=_get_msg(issue, code_messages), path=issue_path, - message=( - f"Consider creating or updating a `.bidsignore` file " - f"in the root of your BIDS dataset to ignore " - f"`{dandiset_metadata_file}`. " - f"Add the following line to `.bidsignore`:\n" - f"{dandiset_metadata_file}" - ), ) - results.append(hint) + ) return results diff --git a/dandi/files/bids.py b/dandi/files/bids.py index a441e5999..a6be86b45 100644 --- a/dandi/files/bids.py +++ b/dandi/files/bids.py @@ -13,10 +13,15 @@ from .bases import GenericAsset, LocalFileAsset, NWBAsset from .zarr import ZarrAsset -from ..consts import ZARR_MIME_TYPE +from ..consts import ZARR_MIME_TYPE, dandiset_metadata_file from ..metadata.core import add_common_metadata, prepare_metadata from ..misctypes import Digest -from ..validate_types import ValidationResult +from ..validate_types import ( + ORIGIN_VALIDATION_DANDI_LAYOUT, + Scope, + Severity, + ValidationResult, +) BIDS_ASSET_ERRORS = ("BIDS.NON_BIDS_PATH_PLACEHOLDER",) BIDS_DATASET_ERRORS = ("BIDS.MANDATORY_FILE_MISSING_PLACEHOLDER",) @@ -114,6 +119,39 @@ def _validate(self) -> None: # deno-compiled BIDS validator self._dataset_errors = bids_validate(self.bids_root) + # Add HINT for dandiset.yaml errors + # If any error is about the dandiset metadata file, add a HINT + # suggesting to add it to .bidsignore + additional_hints = [] + for result in self._dataset_errors: + is_dandiset_yaml_error = ( + result.path is not None + and result.path.relative_to(self.bids_root).as_posix() + == dandiset_metadata_file + ) + if is_dandiset_yaml_error: + hint = ValidationResult( + id="DANDI.BIDSIGNORE_DANDISET_YAML", + origin=ORIGIN_VALIDATION_DANDI_LAYOUT, + scope=Scope.DATASET, + origin_result=result, + severity=Severity.HINT, + dandiset_path=result.dandiset_path, + dataset_path=result.dataset_path, + path=result.path, + message=( + f"Consider creating or updating a `.bidsignore` file " + f"in the root of your BIDS dataset to ignore " + f"`{dandiset_metadata_file}`. " + f"Add the following line to `.bidsignore`:\n" + f"{dandiset_metadata_file}" + ), + ) + additional_hints.append(hint) + + # Add hints to the dataset errors + self._dataset_errors.extend(additional_hints) + # Categorized validation results related to individual assets by the # path of the asset in the BIDS dataset self._asset_errors = defaultdict(list) From f56616baa3aa7b5539904165705b47834827d917 Mon Sep 17 00:00:00 2001 From: Isaac To Date: Thu, 15 Jan 2026 23:23:14 -0800 Subject: [PATCH 5/6] fix: correct the condition for adding `dandiset.yaml` hint and make hint follows respective error --- dandi/files/bids.py | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/dandi/files/bids.py b/dandi/files/bids.py index a6be86b45..7e43163ec 100644 --- a/dandi/files/bids.py +++ b/dandi/files/bids.py @@ -117,19 +117,22 @@ def _validate(self) -> None: # Obtain BIDS validation results of the entire dataset through the # deno-compiled BIDS validator - self._dataset_errors = bids_validate(self.bids_root) + v_results = bids_validate(self.bids_root) - # Add HINT for dandiset.yaml errors - # If any error is about the dandiset metadata file, add a HINT - # suggesting to add it to .bidsignore - additional_hints = [] - for result in self._dataset_errors: - is_dandiset_yaml_error = ( + # Validation results from the deno BIDS validator with an additional + # hint, represented as a `ValidationResult` object, following + # each `dandiset.yaml` error, suggesting to add the `dandiset.yaml` file + # to `.bidsignore`. + v_results_extended: list[ValidationResult] = [] + + for result in v_results: + v_results_extended.append(result) + if ( result.path is not None - and result.path.relative_to(self.bids_root).as_posix() + and result.dataset_path is not None + and result.path.relative_to(result.dataset_path).as_posix() == dandiset_metadata_file - ) - if is_dandiset_yaml_error: + ): hint = ValidationResult( id="DANDI.BIDSIGNORE_DANDISET_YAML", origin=ORIGIN_VALIDATION_DANDI_LAYOUT, @@ -147,10 +150,9 @@ def _validate(self) -> None: f"{dandiset_metadata_file}" ), ) - additional_hints.append(hint) + v_results_extended.append(hint) - # Add hints to the dataset errors - self._dataset_errors.extend(additional_hints) + self._dataset_errors = v_results_extended # Categorized validation results related to individual assets by the # path of the asset in the BIDS dataset From 993be9477d2a0dddd76da22d9cc8158ab7951ecf Mon Sep 17 00:00:00 2001 From: Isaac To Date: Fri, 16 Jan 2026 18:43:43 -0800 Subject: [PATCH 6/6] test: update test to ensure hint for `dandiset.yaml` --- dandi/tests/test_validate.py | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/dandi/tests/test_validate.py b/dandi/tests/test_validate.py index 703de8730..edbf4ae8f 100644 --- a/dandi/tests/test_validate.py +++ b/dandi/tests/test_validate.py @@ -114,30 +114,18 @@ def mock_bids_validate(*args: Any, **kwargs: Any) -> list[ValidationResult]: assert err.dataset_path is not None assert err.path.relative_to(err.dataset_path).as_posix() == dandiset_metadata_file - # The error message should be the original BIDS error, not modified - assert err.message is not None - # We just check that it's about dandiset.yaml, not checking exact message - # since it comes from BIDS validator - - # Check that there is also a HINT about .bidsignore - validation_hints = [ - r - for r in validation_results - if r.severity is not None and r.severity == Severity.HINT - ] - - # Assert that there is at least one hint - assert len(validation_hints) >= 1 - - # Find the hint about .bidsignore for dandiset.yaml - bidsignore_hint = next( - (h for h in validation_hints if h.id == "DANDI.BIDSIGNORE_DANDISET_YAML"), - None, - ) - assert bidsignore_hint is not None - assert bidsignore_hint.message is not None - assert ".bidsignore" in bidsignore_hint.message - assert dandiset_metadata_file in bidsignore_hint.message + # === Assert that there is the dandiset.yaml hint === + i = None + for i, r in enumerate(validation_results): + if r is err: + break + + assert i is not None + # There must be at least one more result after the error + assert len(validation_results) > i + 1 + # The next result must be the hint re: dandiset.yaml + assert validation_results[i + 1].id == "DANDI.BIDSIGNORE_DANDISET_YAML" + assert validation_results[i + 1].severity == Severity.HINT def test_validate_bids_onefile(bids_error_examples: Path, tmp_path: Path) -> None: