Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make RemoteDandiset.get_version() return a VersionInfo instance with validation error fields #1210

Merged
merged 3 commits into from
Feb 8, 2023
Merged

Make RemoteDandiset.get_version() return a VersionInfo instance with validation error fields #1210

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
05596c3
Make `RemoteDandiset.get_version()` return a `VersionInfo` instance w…
jwodder Feb 8, 2023
9a36d21
Update dandi/dandiapi.py
jwodder Feb 8, 2023
f1d752b
Rename & document (Remote)ValidationError
jwodder Feb 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 34 additions & 17 deletions dandi/dandiapi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,23 @@ def __str__(self) -> str:
return self.identifier


class ValidationError(APIBase):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class ValidationError(APIBase):
class RemoteValidationError(APIBase):
"""
.. versionadded:: 0.49.0
Validation Error record obtained from a server. To not to be confused with
:class:`dandi.validate_types.ValidationResult` which provides richer representation
of validation errors.

as most of the constructs in this file have Remote in them. (note that sphinx ref to class might be wrong, typing from memory of how those are done)

But I also wonder if may be ValidationResult could just be used here instead?

Copy link
Member

Choose a reason for hiding this comment

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

if switch to ValidationResult I would just keep all errors (for version and assets) in a single list but with a proper scope annotation within each ValidationResult.

Copy link
Member Author

Choose a reason for hiding this comment

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

if switch to ValidationResult I would just keep all errors (for version and assets) in a single list

That's not really up to us; the API returns the errors in two separate fields, and it wouldn't be worth the hassle to change that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done: f1d752b

Copy link
Member

Choose a reason for hiding this comment

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

well, we have those super-powers I would say, but ok -- let's stay close to the original return structure at this level.

field: str
message: str


class VersionInfo(Version):
"""
.. versionadded:: 0.49.0

Version information for a Dandiset, including information about validation
errors
"""

asset_validation_errors: List[ValidationError]
version_validation_errors: List[ValidationError]


class RemoteDandisetData(APIBase):
"""
Class for storing the data for a Dandiset retrieved from the API.
Expand Down Expand Up @@ -876,15 +893,20 @@ def get_versions(self, order: Optional[str] = None) -> Iterator[Version]:
except HTTP404Error:
raise NotFoundError(f"No such Dandiset: {self.identifier!r}")

def get_version(self, version_id: str) -> Version:
def get_version(self, version_id: str) -> VersionInfo:
"""
Get information about a given version of the Dandiset. If the given
version does not exist, a `NotFoundError` is raised.

.. versionchanged:: 0.49.0

This method now returns a `VersionInfo` instance instead of just a
`Version`.
"""
try:
return Version.parse_obj(
return VersionInfo.parse_obj(
self.client.get(
f"/dandisets/{self.identifier}/versions/{version_id}/info"
f"/dandisets/{self.identifier}/versions/{version_id}/info/"
)
)
except HTTP404Error:
Expand Down Expand Up @@ -958,26 +980,21 @@ def wait_until_valid(self, max_time: float = 120) -> None:
lgr.debug("Waiting for Dandiset %s to complete validation ...", self.identifier)
start = time()
while time() - start < max_time:
try:
r = self.client.get(f"{self.version_api_path}info/")
except HTTP404Error:
raise NotFoundError(
f"No such version: {self.version_id!r} of Dandiset {self.identifier}"
)
if "status" not in r:
# Running against older version of dandi-api that doesn't
# validate
return
if r["status"] == "Valid":
v = self.get_version(self.version_id)
if v.status is VersionStatus.VALID:
return
sleep(0.5)
# TODO: Improve the presentation of the error messages
about = {
"asset_validation_errors": r.get("asset_validation_errors"),
"version_validation_errors": r.get("version_validation_errors"),
"asset_validation_errors": [
e.json_dict() for e in v.asset_validation_errors
],
"version_validation_errors": [
e.json_dict() for e in v.asset_validation_errors
jwodder marked this conversation as resolved.
Show resolved Hide resolved
],
}
raise ValueError(
f"Dandiset {self.identifier} is {r['status']}: {json.dumps(about, indent=4)}"
f"Dandiset {self.identifier} is {v.status.value}: {json.dumps(about, indent=4)}"
)

def publish(self, max_time: float = 120) -> "RemoteDandiset":
Expand Down
6 changes: 6 additions & 0 deletions docs/source/modref/dandiapi.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ Dandisets
:inherited-members: BaseModel
:exclude-members: Config, JSON_EXCLUDE

.. autoclass:: VersionInfo()
:show-inheritance:

.. autoclass:: ValidationError()
:inherited-members: BaseModel

Assets
------

Expand Down