search

dandi / dandi-schema

Schemata for DANDI archive project
Apache License 2.0
7 stars 10 forks source link

Draft: Relax timestamp timezone validation for startDate and endDate fields on NWB file formats #242

Closed aaronkanzer closed 6 months ago

aaronkanzer commented 6 months ago

Relates to https://github.com/dandi/dandi-archive/issues/1944

and changes made for NWB in https://github.com/NeurodataWithoutBorders/pynwb/pull/1886

Observed in failed dandiset validation in staging environment here: https://gui-staging.dandiarchive.org/dandiset/213840

Cc @candleindark @yarikoptic @satra @bendichter

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.59%. Comparing base (e135307) to head (40d8fb5).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #242 +/- ## ========================================== + Coverage 87.42% 91.59% +4.17% ========================================== Files 16 16 Lines 1726 1726 ========================================== + Hits 1509 1581 +72 + Misses 217 145 -72 ``` | [Flag](https://app.codecov.io/gh/dandi/dandi-schema/pull/242/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/dandi/dandi-schema/pull/242/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi) | `91.59% <100.00%> (+4.17%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

satra commented 6 months ago

@aaronkanzer - i'm not sure this is necessary. i believe pydantic default validator for datetime works fine. at least on the example ben sent.

aaronkanzer commented 6 months ago

@aaronkanzer - i'm not sure this is necessary. i believe pydantic default validator for datetime works fine. at least on the example ben sent.

Hmmm, agreed -- accordingly to the docs it should work fine (e.g. pydantic handling different formats of datetime), but it is still failing 🤔

Perhaps a different root cause....looking further...

bendichter commented 6 months ago

Yes, this is what has been confusing us. We don't see any logic in the dandi schema that would seem to require timezones for this field

aaronkanzer commented 6 months ago

Closed for now -- as it doesn't seem that this is the root cause error of validation for certain datetime timestamps.