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.

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

Validation Errors when InjectionMaterials is combination of models #842

Closed
mekhlakapoor opened this issue Mar 15, 2024 · 4 comments
Closed
Assignees

Comments

@mekhlakapoor
Copy link
Contributor

Describe the bug
When injection_materials contains both ViralMaterial and NonViralMaterial objects, the injection fails validation due to "extra input" and "field required" errors. It is incorrectly validating ViralMaterial objects against NonViralMaterials and vice versa.

To Reproduce

  1. Create a ViralMaterial with some fields (ex: titer)
  2. Create a NonViralMaterial with some fields (ex: concentration)
  3. Create an Injection with injection_materials = [viral, nonviral]
from aind_data_schema.core.procedures import TarsVirusIdentifiers, NanojectInjection, ViralMaterial, NonViralMaterial
ids = TarsVirusIdentifiers.model_construct(virus_tars_id="AiV123", prep_lot_number="lot123")
viral = ViralMaterial.model_construct(name="example", tars_identifiers=ids, titer=7000000)
non_viral = NonViralMaterial.model_construct(concentration=0.5)
inj = NanojectInjection.model_construct(injection_materials=[viral,non_viral]
 inj.__class__.model_validate(inj.model_dump())
  1. See validation errors.
Screen Shot 2024-03-15 at 1 31 42 PM

Expected behavior
injection_materials is a union of ViralMaterial and NonViralMaterial. It should allow and validate correctly.

Desktop (please complete the following information):

  • OS: [e.g. iOS]
  • Browser [e.g. chrome, safari]
  • Version [e.g. 22]

Smartphone (please complete the following information):

  • Device: [e.g. iPhone6]
  • OS: [e.g. iOS8.1]
  • Browser [e.g. stock browser, safari]
  • Version [e.g. 22]

Additional context
Add any other context about the problem here.

@bruno-f-cruz
Copy link
Collaborator

bruno-f-cruz commented Mar 16, 2024

You are forcing the model to be built without validation by calling the model_construct method. See the relevant documentation here.

The trace you get is not from the final NanojectInjection per se, but instead because of the recursive validation of the NonViralMaterial. Here's a working example that should get you started:

from aind_data_schema.core.procedures import TarsVirusIdentifiers, NanojectInjection, ViralMaterial, NonViralMaterial
from aind_data_schema.models.organizations import Organization

ids = TarsVirusIdentifiers(virus_tars_id="AiV123", prep_lot_number="lot123")
viral = ViralMaterial(name="example", tars_identifiers=ids, titer=7000000)
non_viral = NonViralMaterial(name="example", source=Organization.ABCAM, rrid=None, lot_number="foo", expiration_date=None)
inj = NanojectInjection(injection_materials=[viral, non_viral],
                        injection_coordinate_ml=0, injection_coordinate_ap=0, injection_coordinate_depth=[0, +10, -10],
                        protocol_id="bar", injection_angle=0, injection_volume=[-10, 0, 0])
inj.model_validate(inj.model_dump())

@jtyoung84
Copy link
Collaborator

This signature is incorrect and may fix the bug if we fix it:

injection_materials: Annotated[
        List[Union[ViralMaterial, NonViralMaterial]],
        Field(title="Injection material", min_length=1, discriminator="material_type"),
    ]

@micahwoodard
Copy link
Contributor

I think this bug might have already been fixed? I can't reproduce the bug when using Bruno's example and injection_materials already has the signature Jon suggested. Let me know if I'm missing something!

@mekhlakapoor
Copy link
Contributor Author

Closing this issue because it works with Bruno's example. Opening issue in metadata-service to fix construction of models to not use model_construct

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants