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

Unified validation check for dependency fields #1348

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

dbochkov-flexcompute
Copy link
Contributor

After playing a little bit with it, managed to implement what we wanted as a convenient decorator. The usage as follows: instead of doing something like

@pydantic.validator("monitors", always=True)
def _warn_monitor_simulation_frequency_range(cls, val, values):
    """Warn if any DFT monitors have frequencies outside of the simulation frequency range."""

    if "sources" not in values:
        raise ValidationError(
            "could not validate `_warn_monitor_simulation_frequency_range` "
            "as `sources` failed validation"
        )

    ...

we can just do

@pydantic.validator("monitors", always=True)
@check_previous_fields_validation(["sources"])
def _warn_monitor_simulation_frequency_range(cls, val, values):
    """Warn if any DFT monitors have frequencies outside of the simulation frequency range."""
    ...

this check whether dependency fields have passed validation, and, if not, it skips (not errors) the current validator and prints something like

WARNING: Could not execute validator                               
'_warn_monitor_simulation_frequency_range' because field 'sources' 
failed validation.

@momchil-flex
Copy link
Collaborator

Why would we only warn instead of error? It seems to me that if previous fields failed validation we should error with the validation error of that field.

@momchil-flex
Copy link
Collaborator

Also I think this PR should go to develop so that we patch this asap.

Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @dbochkov-flexcompute, I think this will be great to have. A couple minor comments and also there are probably many instances here and there where we can implement it already, especially in Simulation. For example these lines.

I wonder if, more generally, we can assume that if any values.get(key) or values[key] is present in a field validator, that we should probably @check_previous_fields_validation for that key.

Another pretty minor comment, perhaps the name of the decorator could be more specific, like

skip_if_validation_failed(fields: List[str])

because check is maybe a bit ambiguous as to what the decorator is doing. (Just a minor suggestion).

def _validator(cls, val, values):
"""New validator function."""
for field in required_fields:
if field not in values:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to confirm, if field fails validation, it's just not put in values? (as opposed to put into values with None?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, just double checked with pydantic documentation https://docs.pydantic.dev/1.10/usage/validators/:

If validation fails on another field (or that field is missing) it will not be included in values, hence if 'password1' in values and ... in this example.

tidy3d/components/base.py Outdated Show resolved Hide resolved
tidy3d/components/simulation.py Outdated Show resolved Hide resolved
@tylerflex
Copy link
Collaborator

Another idea for the name of this validator, maybe more generally could be

skip_if_fields_none(fields: List[str])

@dbochkov-flexcompute
Copy link
Contributor Author

Why would we only warn instead of error? It seems to me that if previous fields failed validation we should error with the validation error of that field.

Just to be clear, the failed field still throws its own error. But I'm ok either way, the difference is very subtle. If we error, then pydantic will collect this additional error and overall we get the following output:

09:47:51 CST ERROR: Automatic grid generation requires the input of 'wavelength'
             or sources.
             ERROR: Could not execute validator 'plane_in_sim_bounds' because   
             field 'simulation' failed validation.
Traceback (most recent call last):
...
pydantic.v1.error_wrappers.ValidationError: 2 validation errors for ModeSolver
simulation -> grid_spec
  Automatic grid generation requires the input of 'wavelength' or sources. (type=value_error.setup)
plane
  Could not execute validator 'plane_in_sim_bounds' because field 'simulation' failed validation. (type=value_error)

Note that pydantic error list contains two errors (pydantic.v1.error_wrappers.ValidationError: 2 validation errors for ModeSolver).

If we just pop a warning, we get

16:58:19 CST ERROR: Automatic grid generation requires the input of 'wavelength'
             or sources.                                                        
             WARNING: Could not execute validator 'plane_in_sim_bounds' because 
             field 'simulation' failed validation.                              
Traceback (most recent call last):
...
pydantic.v1.error_wrappers.ValidationError: 1 validation error for ModeSolver
simulation -> grid_spec
  Automatic grid generation requires the input of 'wavelength' or sources. (type=value_error.setup)

Note that in this case pydantic error list contains just one error (pydantic.v1.error_wrappers.ValidationError: 1 validation error for ModeSolver).

This doesn't really make much difference, but I think when we explicitly error a field ("A") that was not validated just because a previous field ("B") failed validation, this might make an impression that something is wrong with setting up field "A", while in reality it could be that just resolving issues with filed "B" is enough. It might be more relevant for GUI were failed fields are marked visually.

@momchil-flex
Copy link
Collaborator

Oh ok that makes sense thanks!

@momchil-flex momchil-flex changed the base branch from pre/2.6 to develop January 4, 2024 22:28
@dbochkov-flexcompute
Copy link
Contributor Author

revised this PR, bigger changes:

  • rebased to develop
  • per Tyler suggestion to make it more specific renamed the decorator to skip_if_fields_missing
  • added it wherever it seemed to make sense

Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me!

@tylerflex tylerflex self-requested a review January 4, 2024 22:56
@tylerflex
Copy link
Collaborator

thanks @dbochkov-flexcompute

@momchil-flex momchil-flex merged commit e953c00 into develop Jan 5, 2024
14 checks passed
@momchil-flex momchil-flex deleted the daniil/selective-skip-on-failure branch January 5, 2024 00:30
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

Successfully merging this pull request may close these issues.

None yet

3 participants