Converting model models to use the new hydration paradigm #2101
Conversation
github-actions
bot
added
internal
There was a problem hiding this comment.
LGTM from my side but I think you should discuss with @avishniakov which properties exactly to put into body and which into metadata before merging
| mv.to_model_version(suppress_class_validation_warnings=True) | ||
| for mv in model_versions.items | ||
| ] | ||
| for i in range(2, model_versions.total_pages + 1): |
There was a problem hiding this comment.
What is this supposed to do?
It's somehow looping over the amount of pages but using the model versions of the first page?
There was a problem hiding this comment.
Ah yes, I remember that from the old PR, somehow I missed it this time. I think the goal was to depaginate (in which case pagination_utils.depaginate should be used instead)
There was a problem hiding this comment.
What is the plan for this, I'm fine creating a separate PR but that just runs an additional set of tests. I think we should just clarify with @avishniakov what this is supposed to do and fix it. If this should just return a list of all model versions (not paginated), then we simply use pagination_utils.depaginate(...) and convert the results to ModelVersion, should be an easy fix no?
There was a problem hiding this comment.
Yes, this can be substituted with pagination_utils.depaginate 👍🏼
Describe changes
I refactored the model models to utilize the new base models and the hydration paradigm.
Pre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes