Overhauling validation results to get them closer to cover different types of validators

[yarikoptic]

Ref:

• Please submit pointers to your validator output formats con/validation#1

and result of our chat with @candleindark while also reviewing results of linkml validation over dandisets:

• https://github.com/dandi/dandisets-linkml-status/blob/temp-with-error-locations/dandi-reports/summary.md

Notes/TODOs:

• BREAKING CHANGE:
  • we replaced ValidationOrigin.bids_version to more generic Origin.standard + Origin.standard_version
  • We renamed ValidationOrigin.name to Origin.validator and ValidationOrigin.version to Origin.validator_version
  • Severity.HINT, Severity.WARNING, and Severity.ERROR now have value of 20, 30, and 40 respectively instead of 1, 2, and 3.
  • Introduce Origin.type to indicate the type of the origin of a validation result.
  • We made the ValidationResult model into a Pydantic model so that it is readily to serialization to JSON.
• going through the code base and harmonizing construction of those ValidationResults
• redo Validation results refactoring: centralize results for different reused origins, use _pydantic_errors_to_validation_results more #1176 in here

Please see individual commit messages for more details of the change.

Note:
This PR needs the changes in #1601 to pass the tests. Rebase/merge with master once #1601 is merged into master.

[codecov]

Codecov Report

Attention: Patch coverage is 86.66667% with 14 lines in your changes missing coverage. Please review.

Project coverage is 88.66%. Comparing base (b1815da) to head (cf401cd).
Report is 110 commits behind head on master.

Files with missing lines	Patch %	Lines
dandi/files/zarr.py	27.27%	8 Missing ⚠️
dandi/pynwb_utils.py	60.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1514      +/-   ##
==========================================
+ Coverage   88.54%   88.66%   +0.11%     
==========================================
  Files          78       78              
  Lines       10872    10937      +65     
==========================================
+ Hits         9627     9697      +70     
+ Misses       1245     1240       -5     

Flag	Coverage Δ
unittests	88.66% <86.66%> (+0.11%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[candleindark]

In updating the use of ValidationOrigin, it is not clear to me what should be provided for the standard and standard_version attributes of many ValidationOrigin objects. For example, this is not clear in ValidationOrigin objects in organized.py.

dandi-cli/dandi/organize.py

Line 1122 in 45db66c

origin=ValidationOrigin(name="dandi", version=__version__),

Do we have a dandi standard? If we do, it doesn't seem that we version this standard.

I think we should indeed define an enum class for the supported standard, and also talk about how to retrieve the version of those standards.

[candleindark]

In updating the use of ValidationOrigin, it is not clear to me what should be provided for the standard and standard_version attributes of many ValidationOrigin objects. For example, this is not clear in ValidationOrigin objects in organized.py.

dandi-cli/dandi/organize.py

Line 1122 in 45db66c

origin=ValidationOrigin(name="dandi", version=__version__),

Do we have a dandi standard? If we do, it doesn't seem that we version this standard.

I think we should indeed define an enum class for the supported standard, and also talk about how to retrieve the version of those standards.

We now, with the latest commit, have a subclass of Enum, Standard, to represent a list of standards. Some standard is informal, e.g., Standard.DANDI_LAYOUT and have no versioning. In the case, of a standard with no version, we will just let the standard_version attribute of Origin to default to None.

[candleindark]

@yarikoptic I think a ValidationResult object should represent the result of how well a "data" instance is validated. It should inform area/aspect of invalidity.

dandi-cli/dandi/files/bases.py

Lines 193 to 217 in 386e50d

	except Exception as e:
	if devel_debug:
	raise
	lgr.warning(
	"Unexpected validation error for %s: %s",
	self.filepath,
	e,
	extra={"validating": True},
	)
	return [
	ValidationResult(
	origin=ValidationOrigin(
	name="dandi",
	version=dandi.__version__,
	),
	severity=Severity.ERROR,
	id="dandi.SOFTWARE_ERROR",
	scope=Scope.FILE,
	# metadata=metadata,
	path=self.filepath, # note that it is not relative .path
	message=f"Failed to read metadata: {e}",
	# TODO? dataset_path=dataset_path,
	dandiset_path=self.dandiset_path,
	)
	]

The above snippet uses ValidationResult to represent an error other than a ValidationError. The captured error doesn't tell how valid a data instance is. It has nothing to do with the data being validated against but how our software is written. If that error is raised it is either a bug in the software, which should be propagated, or a user's mistake, which should be handled gracefully in another manner. The current arrangement is conflating error handling with the purpose of ValidationResult. Case in point, you can't provide the standard for the origin field in the above example.

Another misuse of ValidationResult is below.

dandi-cli/dandi/pynwb_utils.py

Lines 404 to 427 in 386e50d

	try:
	version = get_nwb_version(path, sanitize=False)
	except Exception:
	# we just will not remove any errors, it is required so should be some
	pass
	else:
	if version is not None:
	# Explicitly sanitize so we collect warnings.
	nwb_errors: list[str] = []
	version = _sanitize_nwb_version(version, log=nwb_errors.append)
	for e in nwb_errors:
	errors.append(
	ValidationResult(
	origin=ValidationOrigin(
	name="pynwb",
	version=pynwb.__version__,
	),
	severity=Severity.ERROR,
	id="pynwb.GENERIC",
	scope=Scope.FILE,
	path=Path(path),
	message=e,
	)
	)

The code snippet above also has other problems. For example, it suppresses errors. The problems should be fixed in an other dedicated PR though.

[candleindark]

This PR needs the changes in #1601 to pass the tests. It will be merged with master once #1601 is merged into master.

[candleindark]

The Change from #1601 is merged. This PR is ready for a final review.

[yarikoptic]

a few spots where to define local origin to be reused.

But also I wonder if generally we should pre-define all those origins so they get reused across all the paths. It smells a little as premature optimization to me though, so may be not worth it. But at least I think it is worth renaming them locally so they become a little more descriptive than just origin variables

[candleindark]

This one is ready for a final review.

[yarikoptic]

FTR: The following look very problematic though not directly related to this PR.

dandi-cli/dandi/pynwb_utils.py

Lines 406 to 408 in 386e50d

	except Exception:
	# we just will not remove any errors, it is required so should be some
	pass

I don't know its intent either.

yeah, better to be reanalyzed/improved. I think we burnt there a few times already

[yarikoptic]

hm, apparently we have no single test on validation of zarrs? that's odd :-/

filed dedicated

• Improve testing of validation #1605

for that

[yarikoptic]

Thank you @candleindark , let's proceed with continuation in

• Improve testing of validation #1605

[github-actions]

🚀 PR was released in 0.68.0 🚀