Fix type signatures to be compatible with pydantic 2.9

[bruno-f-cruz]

Attempted to close #1050. However, the remaining error is really hard to fix at this point without major refactors to the unittest code base.

[bruno-f-cruz]

The final 2 failures I am not sure what is going on as I am not familiar what the tests and what they should be testing for. Perhaps someone can take a look?

Some files that I didn't touch are failing linting on my side:
./aind-data-schema\src\aind_data_schema\core\quality_control.py

[bruno-f-cruz]

The bug seems related with using model_construct (we should really try not to use this method... :P) to get empty classes. If a class has a required field the model construct will omit it from the output (In this case ViralMaterial and its respective name property`). This doesn't seem to be the whole story tho, as trying to reproduce this with a barebones example doesn't work...

Here's a minimal example that reproduces the problem:

from aind_data_schema.core.procedures import ViralMaterial, IontophoresisInjection

viral_material = ViralMaterial.model_construct()
ionto_inj = IontophoresisInjection.model_construct(injection_materials=[viral_material])

ionto_inj.model_dump()

[dbirman]

I'm looking at this now and while I understand what the tests are supposed to do I think you're right the issue might be coming from the use of model_construct all over the place? But fixing that would probably mean instantiating valid objects which seems like a pain as well. I'll look into pulling some valid objects from existing metadata perhaps.

[bruno-f-cruz]

I'm looking at this now and while I understand what the tests are supposed to do I think the issue might be coming from the use of model_construct all over the place? But fixing that would probably mean instantiating valid objects which seems like a pain as well. I'll look into pulling some valid objects from existing metadata perhaps.

Yep, as i said before (#1033), relying on model_construct is probably not the best practice to follow, especially when combined with tagged unions. Tagged unions rely on a lot of behind-the-scenes validation that is skipped during model_construct, making their output somewhat brittle.

Regardless, the current PR should still be merged since it fixes other flagged bugs.

[dbirman]

Yes but there are still tests failing if I force pydantic to 2.9, so I'm trying to track those down

[dbirman]

Looping @mekhlakapoor and @jtyoung84 here, can you help me understand what this test is doing and why it uses model_construct? Does this actually need to build a full Metadata object just to check whether injection_materials is missing?:

        # Tests missing injection materials
        surgery2 = Surgery.model_construct(procedures=[nano_inj])
        with self.assertRaises(ValueError) as context:
            Metadata(
                name="ecephys_655019_2023-04-03_18-17-09",
                location="bucket",
                data_description=DataDescription.model_construct(
                    label="some label", platform=Platform.ECEPHYS, creation_time=time(12, 12, 12)
                ),
                subject=Subject.model_construct(),
                procedures=Procedures.model_construct(subject_procedures=[surgery2]),
                rig=Rig.model_construct(),
                processing=Processing.model_construct(),
                session=Session.model_construct(),
            )
        self.assertIn("Injection is missing injection_materials.", str(context.exception))

If this is just testing whether injection_materials is missing, can I just replace it with a test that does

        with self.assertRaises(ValidationError) as context:
            NanojectInjection(
                    protocol_id="5678",
                    recovery_time=0,
                    instrument_id=None,
                    injection_hemisphere="Left",
                    injection_coordinate_ml=-0.87,
                    injection_coordinate_ap=-3.8,
                    injection_coordinate_depth=[-3.3],
                    injection_coordinate_reference="Lambda",
                    bregma_to_lambda_distance=4.1,
                    injection_angle=10,
                    injection_volume=[200],
                    targeted_structure="VISp",
                )
        self.assertIn("1 validation error for NanojectInjection\ninjection_materials", str(context.exception))

Or even simpler just do the self.assertRaises?

The use of model_construct inside of these deep classes seems to cause new issues in Pydantic 2.9 and I think we need to avoid it when possible, and in general test with "real" objects.