Add strain field to subject metadata and corresponding test case

[bendichter]

fix #1715

[codecov]

Codecov Report

❌ Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.82%. Comparing base (2827711) to head (b0b07c6).
⚠️ Report is 125 commits behind head on master.

Files with missing lines	Patch %	Lines
dandi/pytest_plugin.py	66.66%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1716   +/-   ##
=======================================
  Coverage   74.82%   74.82%           
=======================================
  Files          84       84           
  Lines       11693    11701    +8     
=======================================
+ Hits         8749     8755    +6     
- Misses       2944     2946    +2     

Flag	Coverage Δ
unittests	74.82% <70.00%> (+<0.01%)	⬆️

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.

[bendichter]

@yarikoptic can you take it from here?

[yarikoptic]

Thank you @bendichter ! Trying to figure it out but goes slow -- what I can say only that it seems that it does the job but we do need some better way to depict what pytest found different in that bloody comparison...

[yarikoptic]

@candleindark if you see how to improve this -- the goal is to make it easier to understand diffs on our pydantic models. If you see how -- please try your idea first before suggesting since there could be "gotchas" ;-)

[candleindark]

Instead of

ldict, rdict = dict(left), dict(right)

we can

ldict, rdict = left.model_dump(), right.model_dump()

so we can get all the sub-models recursively converted to dictionaries.

Beyond that, we can use library like https://zepworks.com/deepdiff/8.6.1/diff.html and https://github.com/xlwings/jsondiff to locate the exact difference in the nested structure.

[yarikoptic]

I believe model dump doesn't work here, hence I suggested to try first ;-)

[candleindark]

After digging into it a bit more. Here is what I found.

The reason model_dump() doesn't work is because the right operant is not a valid DandiBaseModel instance and contains values such as anys.ANY_AWARE_DATETIME, which is not serializable Pydantic by default. The right operate is constructed by calling the model_construct method to bypass the validation.

One way to allow a meaningful comparison illustration of the two operants is to make model_dump() to work on the right operant as well. This can be accomplished by using a custom serializer to the dandischema.models.DandiBaseModel. This custom serializer is depicted in the simplified model below.

Custom serializer example

from typing import Any

from anys import ANY_AWARE_DATETIME, AnyBase

from datetime import datetime


from pydantic import BaseModel, field_serializer, SerializerFunctionWrapHandler


class Foo(BaseModel):
    date: datetime = datetime.fromisoformat("2000-01-01")

    @field_serializer("*", mode="wrap")
    def ignore_any_types(
        self, value: Any, handler: SerializerFunctionWrapHandler
    ) -> Any:
        if isinstance(value, AnyBase):
            return value
        return handler(value)


foo = Foo.model_construct(date=ANY_AWARE_DATETIME)
print(foo.model_dump())

Once this custom serializer is added to dandischema.models.DandiBaseModel. We can change the line https://github.com/dandi/dandi-cli/blob/28d954c7620f80d96fc749dcd39410acdbbd10d9/tox.ini#L16C5-L16C46

to

coverage run -m pytest -vv {posargs} dandi

to see the detailed difference in the structure of the objects.

Note This solution involves changing the dandi-schema repo, including adding the anys dependency to the dandischema pacakge.

@yarikoptic Please let me know if you want to proceed with this solution. I will send a PR if yes.

[yarikoptic]

Since you did this analysis already, I think it might well be with adding. But I wonder if we could avoid need too depend on any. Presumably we can get instance only if any already installed, at we can easily just try to import any and if gains to import just not to do that check.

To fix this issue, we should remove the redundant and always-failing assertion assert ldict == rdict on line 67. This assertion is unnecessary, because the logic already branches appropriately depending on whether the dictionaries are equal, and the function's documentation/comment notes that pytest will "recurse" into its own comparison machinery if the dicts differ. By deleting this line, we avoid dead code and potential confusion from inevitable, spurious assertion errors. We do not need to add imports or change any other logic. Only remove line 67 from dandi/pytest_plugin.py.

---

[candleindark]

Instead of

ldict, rdict = dict(left), dict(right)

we can

ldict, rdict = left.model_dump(), right.model_dump()

so we can get all the sub-models recursively converted to dictionaries.

Beyond that, we can use library like https://zepworks.com/deepdiff/8.6.1/diff.html and https://github.com/xlwings/jsondiff to locate the exact difference in the nested structure.

[bendichter]

@yarikoptic what's going on with that one single test on ubuntu python 3.11?

[yarikoptic]

FTR, it was

FAILED dandi/cli/tests/test_service_scripts.py::test_reextract_metadata - Failed: Timeout (>300.0s) from pytest-timeout.

I have restarted to see if that was a fluke (as it should)

[bendichter]

I don't think it's a fluke. I think it has happened multiple times

[github-actions]

🚀 PR was released in 0.72.0 🚀