Conversation
980502c to
b93f341
Compare
Sukh-P
left a comment
There was a problem hiding this comment.
Looks good, good spots and thanks for fixing this!
Had one small comment but not holding up merging, it's that the first edge case you described doesn't feel well tested still about nwp_encoders_dict always being present but sometimes being None. I am confident your changes will work for it but I guess the test coverage around that edge case still feels low
That is true. We would have caught this before if we had that. I don't want that to hold up this PR though. So maybe it can be its own issue |
Pull Request
Description
There are a few edge case bugs in the validation function that this PR fixes.
e.g.
if hasattr(model, "nwp_encoders_dict"):which always evaluate to true since the model always hasnwp_encoders_dicteven if it is empty.Plus some other light refactoring and clean ups
Checklist: