Skip to content

Improve use of asset validation errors#2719

Draft
jjnesbitt wants to merge 13 commits into
masterfrom
zarr-asset-validation-error
Draft

Improve use of asset validation errors#2719
jjnesbitt wants to merge 13 commits into
masterfrom
zarr-asset-validation-error

Conversation

@jjnesbitt

@jjnesbitt jjnesbitt commented Feb 26, 2026

Copy link
Copy Markdown
Member

The primary purpose of this PR is to address an issue with asset validation errors for zarr assets. The existing behavior is to share the same status between assets that are in the process of validation, and zarr assets that cannot be validated yet (due to the underlying zarr not being finalized).

This is observed mostly on dandiset 000108, however other dandisets have experienced this as well. This change won't actually address the underlying problem, as the solution to that is simply to finalize the zarrs backing those assets (something I will address separately). However, it will fix the asset validation error that is shown, to more accurately describe the problem.

There are several secondary improvements:

  • Remove the deprecated VALIDATING status on the Asset model
  • Fix the broken asset validation UI
  • Separate out asset validation errors into their own endpoint, removing them from the version info endpoint.
  • Enabled by the above improvement, remove the truncation of asset validation errors (originally introduced in Truncate asset validation errors/fix memory usage #2548)

@jjnesbitt jjnesbitt added patch Increment the patch version when merged release Create a release when this pr is merged labels Feb 26, 2026
@jjnesbitt jjnesbitt force-pushed the zarr-asset-validation-error branch 2 times, most recently from fa4467e to 7ee65b2 Compare February 26, 2026 23:13
@jjnesbitt

Copy link
Copy Markdown
Member Author

I still need to address CLI integration...

Comment thread dandiapi/api/models/asset.py
Comment thread dandiapi/api/models/version.py Outdated
Comment thread dandiapi/api/models/version.py Outdated
@jjnesbitt jjnesbitt force-pushed the zarr-asset-validation-error branch from 7ee65b2 to 112b08c Compare March 2, 2026 23:12
@jjnesbitt jjnesbitt force-pushed the zarr-asset-validation-error branch from 112b08c to 96562a3 Compare March 2, 2026 23:15
if: matrix.dandi-version == 'master'
run: >
uvx --with "dandi[test] @ git+https://github.com/dandi/dandi-cli"
uvx --with "dandi[test] @ git+https://github.com/dandi/dandi-cli@use-new-asset-validation-errors"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not be merged with this AFAIK... I wish there was a comment about PR which to check if this branch was merged/released

Suggested change
uvx --with "dandi[test] @ git+https://github.com/dandi/dandi-cli@use-new-asset-validation-errors"
uvx --with "dandi[test] @ git+https://github.com/dandi/dandi-cli"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yarikoptic yarikoptic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove testing against some dev branch of dandi-cli

@jjnesbitt

Copy link
Copy Markdown
Member Author

This PR is still blocked on CLI integration. I do not have time to address this in the short term, so I will put it back into draft.

@jjnesbitt jjnesbitt marked this pull request as draft April 29, 2026 15:23
@yarikoptic

Copy link
Copy Markdown
Member

I do not see 'restart' button somehow but should be ok if redone now (dandi-cli vise)

@yarikoptic

Copy link
Copy Markdown
Member

note: I tuned description to put "Closes #XXX" into itemized list so we could see the titles of those issues to be closed

However, it will fix the asset validation error that is shown, to more accurately describe the problem.

you left me hanging there ;) could you visualize how it looked before in UI and now?

Screenshot for added new endpoint in swagger?

I am yet to grasp why we need a new endpoint (and thus changes to dandi-cli) as we already have per asset one:
image

and not some option for an existing endpoint to filter if so desired for UI -- was such an option considered? (we already have many good options for other endpoints, as eg. filtering in /dandisets/ etc)

@jjnesbitt

jjnesbitt commented Apr 30, 2026

Copy link
Copy Markdown
Member Author

I am yet to grasp why we need a new endpoint (and thus changes to dandi-cli) as we already have per asset one: image

The per asset validation error endpoint is not helpful if you want to view all asset validation errors.

and not some option for an existing endpoint to filter if so desired for UI -- was such an option considered? (we already have many good options for other endpoints, as eg. filtering in /dandisets/ etc)

The dandisets endpoint you mentioned is perhaps the most clunky and inefficient endpoint in the entire API (and to which we've had numerous past bugs), precisely because it attempts to do too many things at once via query params and filtering.

Right now, the asset validation errors are bundled into the version info endpoint, which is used in several ways, and is overall an important endpoint. Since there may be many asset validation errors for a given version, if all errors are returned, this can can cause timeouts or otherwise slow response times. To avoid this, we artificially limit the number of asset validation errors that are returned from the version info endpoint. However, this causes issues, namely an incomplete and misleading picture of what asset validation errors are present on a version.

Separating out the asset validation errors into their own endpoint removes this burden from the main version info endpoint, and allows for the full list of asset validation errors to be returned from the new asset validation errors endpoint, without affecting the return of other data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

patch Increment the patch version when merged release Create a release when this pr is merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

problem rendering many asset validation errors Remove use of Asset VALIDATING status if not used Dandiset info endpoint large response sizes

3 participants