feat: add metadata to Record
#3194
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #3194 +/- ##
===========================================
- Coverage 90.91% 90.68% -0.24%
===========================================
Files 215 215
Lines 11304 11342 +38
===========================================
+ Hits 10277 10285 +8
- Misses 1027 1057 +30
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Co-authored-by: Alvaro Bartolome <alvaro@argilla.io>
| @classmethod | ||
| def from_orm(cls: Type["Record"], obj: Any) -> "Record": | ||
| dict_copy = obj.__dict__.copy() | ||
| return cls(**{"metadata": dict_copy.pop("metadata_", None), **dict_copy}) |
There was a problem hiding this comment.
In the listing records endpoints we were using record.__dict__ to build the Record schema. This is because if we use record directly, then pydantic when creating the Record class will try to access record.responses and sqlalchemy will return an empty list no matter what, so in the output payload we would have "responses": [] even if the client has not asked for the responses.
To fix this, we use record.__dict__ which does not include the responses if they were not included in the query with a joinedload or other load option. Now that we've added the metadata_ attribute, we cannot use record.__dict__ directly to build the Record class, because the field in this class is called metadata and we have to map this manually.
As we had to do this in several endpoints, I've decided that it was a good idea to put this logic in this method.
There was a problem hiding this comment.
Can we use an alias then for the record.metadata field? Or use a RecordGetter class to tackle this? I feel this from_orm override is a bit tricky
There was a problem hiding this comment.
I managed to do the same with the GetterDict and keep the response as unset if the query didn't have the joinedload option. It was as easy as returning the default argument instead of None.
class RecordGetterDict(GetterDict):
def get(self, key: str, default: Any) -> Any:
if key == "metadata":
return getattr(self._obj, "metadata_", None)
if key == "responses" and "responses" not in self._obj.__dict__:
return default
return super().get(key, default)This way pydantic detects that the field has not been set and in combination with the decorator parameter response_model_exclude_unset=True it works.
…om/argilla-io/argilla into feature/api-record-metadata-column
Description
This PR adds a new attribute called
metadatato theRecordof theFeedbackDataset.metadatacolumn/attribute has been added to theRecordORM class (a new columnmetadatawill be added by the migration script generated byalembic)metadatacolumnmetadatakey returned by the APImetadataAdditionally, I've refactor some
if/elseconditions in some methods from theFeedbackDatasetclass.Closes #3155
Type of change
How Has This Been Tested
Unit tests has been updated and I've done some manual tests.
Checklist