Add more testing for missing variables for Channel Settings.

[edyoshikun]

Openning this issue to point and document some errors that we ran during acquisition.

Problem:
In the last experiment, copy/pasting the old yaml file lead to some bugs. The old mantis Yaml files used the channel settings -> time_internal_s vs the new channel settings -> time_interval_s. The slight misspeling does not throw errors and can blindly run the code.

Solution:
Create tests on the pydantic base model so that there are some variables that should be required for the acquisition to run. This ensures the variables match the model's settings names.

[talonchandler]

@edyoshikun and I discussed this with @JoOkuma and found that Optional is being used in an unintended way here (Optional[int] is actually a shorthand for int | None).

For example:

In [1]: from pydantic.dataclasses import dataclass
In [2]: from typing import Optional
In [3]: @dataclass
   ...: class TestSettings:
   ...:     a: Optional[int] = 0
   ...:
In [4]: TestSettings(b=1)
Out[4]: TestSettings(a=0)

is "failing silently" which is the type of error @edyoshikun is seeing here.

An alternative that's closer is:

In [5]: @dataclass
   ...: class TestSettings2:
   ...:     a: int
In [6]: TestSettings2(b=1)
------------------------------------------------------------------
TypeError                        Traceback (most recent call last)
Cell In[7], line 1
----> 1 TestSettings2(b=1)

File /opt/anaconda3/envs/mantis-dev/lib/python3.9/site-packages/pydantic/dataclasses.py:322, in pydantic.dataclasses._add_pydantic_validation_attributes.new_init()

File /opt/anaconda3/envs/mantis-dev/lib/python3.9/site-packages/pydantic/dataclasses.py:286, in pydantic.dataclasses._add_pydantic_validation_attributes.handle_extra_init()

TypeError: __init__() missing 1 required positional argument: 'a'

which gives an error, but this solution requires that you provide a.

If you want a to be optional (i.e. it takes a default value if you don't provide a value) and you want the model to fail if you provide an extra keyword, you can use the following:

In [7]: from pydantic import BaseModel, Extra
In [8]: class TestSettings3(BaseModel, extra=Extra.forbid):
      ...:     a: int = 0
      
In [9] TestSettings3()
Out[9]: TestSettings3(a=0)      
In [9]: TestSettings3(b=0)
------------------------------------------------------------------
ValidationError                  Traceback (most recent call last)
Cell In[22], line 1
----> 1 TestSettings3(b=0)

File /opt/anaconda3/envs/mantis-dev/lib/python3.9/site-packages/pydantic/main.py:341, in pydantic.main.BaseModel.__init__()

ValidationError: 1 validation error for TestSettings3
b
  extra fields not permitted (type=value_error.extra)      
      

[ieivanov]

Sorry, this was a know (to me) issue. The solution, as you figured out, is to create a config which forbids extra args rather than silently ignoring them. Here is more documentation: https://docs.pydantic.dev/latest/usage/model_config/

[talonchandler]

This is an easy fix. I'll push to main, and pull into @ieivanov's autofocus branch #48.

[talonchandler]

I spoke too soon...this turned out to more challenging than I anticipated.

On my initial attempt I looked for a way to forbid extra parameters for the existing pydantic dataclasses. I found that the extra=Extra.forbid keyword described above would not work out of the box because mantis' acquisition configuration classes are pydantic dataclasses, not pydantic BaseModels. Although the documentation makes it seem like these are very similar, they are different and aren't immediately swappable.

Next, I made an attempt to migrate the dataclasss to BaseModels. I ran into roadblocks, particularly related to the many List[DevicePropertySettings] attributes that are provided in the example configs as lists of lists. I found that dataclasses and BaseModels handle these cases very differently---dataclasses smoothly translate a list of lists into a list of DevicePropertySettings, but BaseModels are more strict and don't handle this. Before handling this case, I started to run into other dataclass -> BaseModel translation headaches (e.g. __post_init__ -> @validator, field -> Field).

I am concerned that continuing down this dataclass to BaseModel conversion path will introduce more hardware-dependent bugs (that @ieivanov has already squashed on his first passes) than it'll solve, particularly since I don't have enough coverage to test without the hardware.

@ziw-liu @ieivanov @edyoshikun if you can see a smoother path that leaves the existing work unchanged I'm open to it.

[ziw-liu]

I found that the extra=Extra.forbid keyword described above would not work out of the box because mantis' acquisition configuration classes are pydantic dataclasses, not pydantic BaseModels. Although the documentation makes it seem like these are very similar, they are different and aren't immediately swappable.

When I wrote iohub's NGFF models I started with dataclass, but found that it doesn't provide full functionality and rewrote it with BaseModel. Unfortunately I don't know a better way to solve this.

[ieivanov]

Let me take a quick look also. @talonchandler what branch are you working off of?

[talonchandler]

@ieivanov I put my WIP with some annotations in #72 .

[ieivanov]

@talonchandler I think the difference is in how you apply the extra='forbid' config. Take a look here: https://docs.pydantic.dev/latest/usage/dataclasses/#dataclass-config

I started this work in #73 , you're welcome to pick up from here or transfer these commits to another PR you're working on

[talonchandler]

I closed #72 and opened #73 for review. Thanks again @ieivanov.