Schemafy merge_fixed_preprocessing_params #3223
Conversation
| @@ -37,7 +37,7 @@ class ImagePreprocessingConfig(BasePreprocessingConfig): | |||
| parameter_metadata=FEATURE_METADATA[IMAGE][PREPROCESSING]["computed_fill_value"], | |||
| ) | |||
|
|
|||
| height: int = schema_utils.PositiveInteger( | |||
| height: Optional[int] = schema_utils.PositiveInteger( | |||
There was a problem hiding this comment.
So in the past, we've refrained from putting optional on parameters since it already has a default of None - so if a user doesn't specify a value here, it will just stay as None. This makes adding the Optional typing sort of redundant. Curious if you had a specific reason for adding this in?
There was a problem hiding this comment.
It's not redundant, because the Optional here is primarily useful for static analysis when the config is being used in the Ludwig code. If it's possible for the value to be None, but the type is int, then the expectation from the API user should be that they can treat the value as an int without needing to do any checks for None. But this will lead to them getting NoneType exceptions. Does that make sense?
There was a problem hiding this comment.
+1. Even if it's redundant, should be marked with Optional or Union[..., None]
There was a problem hiding this comment.
Ah I see, that makes sense! In that case, I think there's probably a handful of places in the schema where we would want to add in some Optional typing. Obviously out of scope for this PR but that's good to know going forward. Thanks for the detailed explanation!
Also removes checks on config dict that are covered by existing marshmallow schema validation. These checks were only needed for the merge logic.