Digests for uningested Zarrs should be absent, not `null`

[jwodder]

If a Zarr has not yet been ingested, then currently the .digest."dandi:dandi-zarr-checksum" field of its metadata as returned by the API will be null (example: https://api.dandiarchive.org/api/assets/18498fc4-7d55-4d3d-a19b-7ae837f6b630/ — EDIT: This was an example when I posted it). This is invalid, as the metadata schema requires that digest fields be strings.

CC @yarikoptic: This change will affect how uningested Zarrs are handled by backups2datalad; with the current code, a Zarr that is missing a digest (as opposed to one with a null digest) will be treated like a blob asset missing its SHA256 digest, causing all further assets in the Dandiset to not be backed up.

[yarikoptic]

Thanks for the buzz. zarrs are odd beasts, so IMHO we should behave on them differently, allowing for them to have null (unknown) checksum since they can mutate while we are "backing up". That is what I explicitly "coded for" in that pseudo code in dandisets/000108#13 (comment) .

CC @yarikoptic: This change

you mean that if/when this issue is fixed, i.e. instead of null it would become some (empty? "none"?) string, right? (asking since it is not a PR and not yet clear how it would change)

[jwodder]

@yarikoptic Yes, I mean when this issue is fixed; I expect it to be fixed by null digests simply not being present, which would affect the backup script.

[waxlamp]

@yarikoptic, @jwodder: would it be better to adjust the digest field to be nullable, or to use an empty string instead of null? My gut says to allow for null as an explicit signal that the digest hasn't been computed yet.

[jwodder]

@waxlamp

would it be better to adjust the digest field to be nullable or to use an empty string instead of null?

No. Those would require further changes in dandi-cli, which assumes the digest values are always nonempty strings.

My gut says to allow for null as an explicit signal that the digest hasn't been computed yet.

I think the digest being absent is enough of a signal.

[jjnesbitt]

Conditionally including/excluding fields from the API response is an anti-pattern IMO, and I'd prefer not to go that route unless it's absolutely necessary.

An un-ingested zarr is invalid, until the checksum is processed, and should be treated as such. If we allowed the change you're proposing, one of the consequences would be that un-ingested zarrs wouldn't raise any validation errors on a dandiset, which would incorrectly allow for publishing of those assets (if we could publish zarrs).

How is the backups2datalad script behaving regarding un-ingested zarrs that is problematic? Could the invalid assets not just be skipped?

[jwodder]

@AlmightyYakob backups2datalad is not the problem. The problem is that the API is violating the contract with the user by generating & returning metadata that the API defines as invalid.

[yarikoptic]

@AlmightyYakob could the dialog be continued to resolution one way or another so the issue could be addressed ?

[jjnesbitt]

It seems we have two options:

1. Conditionally hide the checksum field if it's null
2. Modify the backups2datalad to handle the case of a null checksum

I'm opposed to option 1 because it both goes against the design of the rest of the API, and is generally a clumsy design (i.e. a field "sometimes" appearing based just on if it's null or not is confusing and annoying to use). I'm not sure why there's such opposition to modifying how backups2datalad handles this case, but if it really is not an option, I suppose we could change the field. However, I'd like someone to acknowledge the point I made above:

An un-ingested zarr is invalid, until the checksum is processed, and should be treated as such. If we allowed the change you're proposing, one of the consequences would be that un-ingested zarrs wouldn't raise any validation errors on a dandiset, which would incorrectly allow for publishing of those assets (if we could publish zarrs).

This is undesired behavior afaik, and if we're going to address this issue by just pretending that invalid zarrs are valid if they're not ingested, then I'd like some stakeholders to acknowledge that they're okay with this outcome.

[waxlamp]

I agree with Jake's assessment. For purposes of a consistent, easy-to-understand API I believe we should always show the checksum field and use a sentinel value (I would push for null) to indicate a missing checksum.

@jwodder wrote:

The problem is that the API is violating the contract with the user by generating & returning metadata that the API defines as invalid.

In general, our API often returns invalid metadata. For instance, a dandiset with a blank description would cause the metadata API to return metadata that "violates the contract with the user" -- but I would argue there is no such contract. The meditor's own behavior in showing the user where the metadata fails validation is the proof of that statement. I believe any "contract" that exists has more to do with the API refusing the publish a dandiset that has deficiencies of this sort.

In particular, if we go with the idea of hiding the checksum field where there is no checksum, we would still be violating such a contract: a Zarr without a checksum, whether you show a checksum field or not, is invalid. At that point, hiding the field simply makes the API less reliable for human users.

I believe the cost of our always showing the field, regardless of the presence of a checksum, is that downstream scripts would need to refactor to account for the sentinel value indicating missing checksums. Without knowing more, I would imagine this is a small cost, and well worth the benefit of a consistent API response.

In case we're talking past each other at this point, I propose a meeting between @AlmightyYakob, @jwodder, @yarikoptic, and @waxlamp to discuss further. Let me know if that's something we want to do and I can set it up.

[jwodder]

I'm opposed to option 1 because it both goes against the design of the rest of the API, and is generally a clumsy design (i.e. a field "sometimes" appearing based just on if it's null or not is confusing and annoying to use)

From the user's perspective, the field would be either present & a string or else absent. That seems easier to use than one that could also be null.

---

If merely omitting a null digest makes an asset valid when it shouldn't be, then I feel there should be some other, explicit marker of the fact that the Zarr is still being processed, and this marker should be what causes the asset to fail validation. The marker need not be present in the asset metadata, as presumably asset validation can take more than just metadata into account.

---

In general, our API often returns invalid metadata.

And I believe that is a giant mistake, one that I had hoped would be restricted to things like omitting required fields and fields having non-validating values, not fields being completely the wrong type. If DANDI emits metadata and the very tools DANDI provides for working with that metadata (i.e., dandischema) are unable to process it — especially because the server chose to make it unprocessable — then DANDI is at fault. I dream of a world where the Dandi CLI's Python API's get_metadata() methods always work, rather than too often forcing the user to fall back to get_raw_metadata() and muck about with dicts of metadata like a JavaScript programmer, without the benefit of the dandischema classes that we made for this very job.

---

In case we're talking past each other at this point, I propose a meeting between @AlmightyYakob, @jwodder, @yarikoptic, and @waxlamp to discuss further. Let me know if that's something we want to do and I can set it up.

If we have that meeting, then @satra should also be there, as I will be pushing my point about dandischema above and arguing for a proper schema for draft metadata.

[waxlamp]

I'm opposed to option 1 because it both goes against the design of the rest of the API, and is generally a clumsy design (i.e. a field "sometimes" appearing based just on if it's null or not is confusing and annoying to use)

From the user's perspective, the field would be either present & a string or else absent. That seems easier to use than one that could also be null.

My strongly-held opinion is that the programming patterns are equally easy to deploy in both scenarios, but the always-present version is better because then someone viewing a response that doesn't include the checksum wouldn't need special knowledge that something is absent that could be present (instead, they would only need to know what it means for the checksum to be None when the type of that field is string | None).

To be clear, I'm arguing for changing our system so that the type of that field is indeed string | None. However, in the end I would also be fine with leaving the types alone and changing what the server does. Perhaps this is something to finish hashing out when we meet.

In general, our API often returns invalid metadata.

And I believe that is a giant mistake, one that I had hoped would be restricted to things like omitting required fields and fields having non-validating values, not fields being completely the wrong type. If DANDI emits metadata and the very tools DANDI provides for working with that metadata (i.e., dandischema) are unable to process it — especially because the server chose to make it unprocessable — then DANDI is at fault.

To bolster my previous point: we control the types of things, and we can change the type of the checksum field if it gains us something. And to continue to try to be as clear as possible, I do agree with you that we need to respect the types we've chosen to declare for various things. I'm currently arguing for changing the type, while you're arguing for changing the server's behavior. Either one of those would solve the problem and we should figure out which one we like better.

I dream of a world where the Dandi CLI's Python API's get_metadata() methods always work, rather than too often forcing the user to fall back to get_raw_metadata() and muck about with dicts of metadata like a JavaScript programmer, without the benefit of the dandischema classes that we made for this very job.

In case we're talking past each other at this point, I propose a meeting between @AlmightyYakob, @jwodder, @yarikoptic, and @waxlamp to discuss further. Let me know if that's something we want to do and I can set it up.

If we have that meeting, then @satra should also be there, as I will be pushing my point about dandischema above and arguing for a proper schema for draft metadata.

I agree that we need an explicit schema for draft metadata (something @marySalvi is interested in for the archive codebase as well). So let's meet and figure this out 🙂