Make Input, Output, ModelSpec instances immutable.#2537
Make Input, Output, ModelSpec instances immutable.#2537davemfish wants to merge 9 commits intonatcap:mainfrom
Conversation
Added a Parent class for shared config.
…ify during validation. natcap#2228
emlys
left a comment
emlys
left a comment
There was a problem hiding this comment.
Thanks for tackling this @davemfish! I had avoided the immutability question myself when first developing this because of the difficulty with evaluating the required strings. Just a few comments
|
|
||
|
|
||
| class Input(BaseModel): | ||
| class Parent(BaseModel): |
There was a problem hiding this comment.
Could this have a more descriptive name like ImmutableBaseModel?
| # Using deepcopy to make sure we don't modify the original spec | ||
| parameter_spec = copy.deepcopy(model_spec.get_input(key)) | ||
| # Using a deep copy to make sure we don't modify the original spec | ||
| # spec or original nested specs. |
There was a problem hiding this comment.
Is "spec" repeated an extra time in this sentence?
| col_spec.required = bool(utils.evaluate_expression( | ||
| # attributes have faux immutability; we cannot assign | ||
| # to them, but we can assign to the underlying dict. | ||
| col_spec.__dict__['required'] = bool(utils.evaluate_expression( |
There was a problem hiding this comment.
Nice to know that we can get around immutability like this if needed! Would it be worth implementing some sort of copy-with-update function to create a new instance with the changed attribute? Given that it's only used in one place, maybe it's not worth it, but it would be nice to respect immutability if possible.
There was a problem hiding this comment.
Pydantic provides model_copy(update={}) so it's probably worth using in this spot. I made that update. In the similar, but more complicated, situation in validation.py it did not seem worth it to copy and replace all the objects because of how they are nested.
| number=count)) | ||
| header=f'{self.orientation}(s)', | ||
| header_name=','.join(duplicated_columns), | ||
| number='multiple')) |
There was a problem hiding this comment.
This block is unrelated to the rest of the PR, but I noticed these variables were not defined. I think this code basically never runs because if pandas reads a CSV with duplicate column names, it adds a suffix to the names as needed. I think the only way to get here is when the orientation is "rows", but then we transpose the table and rows become columns, then there can be duplicate columns at that point.
There can be multiple duplicate columns, so it makes for a complicated message to try to describe the count of each duplicate. I think it's more helpful to just have the message read:
'Expected the row(s) "1, 2" only once but found it multiple times'
or
'Expected the row(s) "1" only once but found it multiple times'
Still not ideal, but this message is also used elsewhere so I didn't want to change the template.
2 participants
Added a
Parentclass for shared config.There's an interesting case during validation where we do intentionally modify the
requiredattribute because it needs to be evaluated dynamically, before a subsequent call to validation. It seemed cleaner to override the immutability than to create copies of all the nested specs, update them, and copy the parent spec and update it with the new list of nested specs, before calling the parent'svalidatemethod.This only came up for
Inputswith nested specs, because for theInputitself, while it can have arequiredexpression that needs evaluating, there is no need to update the originalrequiredvalue after evaluating the expression.Fixes #2228
Checklist