[ENHANCEMENT] Argilla SDK: Updating record fields and vectors #5026

frascuchon · 2024-06-14T11:27:12Z

Description

This PR reviews the record attributes and normalizes how to work with fields, vectors, and metadata. Now, all are treated as dictionaries and users can update in the same way that working with dictionaries or creating new attributes:

record = Record(fields={"name": "John"})

record.fields.update({"name": "Jane", "age": "30"})
record.fields.new_field = "value"

record.vectors["new-vector"] = [1.0, 2.0, 3.0]
record.vectors.vector = [1.0, 2.0, 3.0]

record.metadata["new-key"] = "new_value"
record.metadata.key = "new_value"

Once this approach is approved, I will create a new PR changing the docs.

Type of change

(Please delete options that are not relevant. Remember to title the PR according to the type of change)

New feature (non-breaking change which adds functionality)
Refactor (change restructuring the codebase without changing functionality)
Improvement (change adding some improvement to an existing functionality)

How Has This Been Tested

(Please describe the tests that you ran to verify your changes. And ideally, reference tests)

Test A
Test B

Checklist

I added relevant documentation
I followed the style guidelines of this project
I did a self-review of my code
I made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
I filled out the contributor form (see text above)
I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

…tainers (api_model method)

burtenshaw · 2024-06-17T07:53:56Z

@frascuchon @davidberenstein1957

Looks nice, just some in line comments and queries.

argilla/src/argilla/records/_dataset_records.py

burtenshaw · 2024-06-17T08:01:54Z

argilla/src/argilla/records/_resource.py

+    def __init__(self, metadata: Optional[Dict[str, MetadataValue]] = None) -> None:
+        super().__init__(metadata or {})
+
+    def __getattr__(self, item: str):


Are we definitely going to support both of these strategies?

record.vectors["new-vector"] = [1.0, 2.0, 3.0] record.vectors.vector = [1.0, 2.0, 3.0] record.metadata["new-key"] = "new_value" record.metadata.key = "new_value"

Personally, I'm not 100% sure about the attribute based approach because I don't like how the IDE behaves. Is there a way to improve the experience there?

I think it is clearer to only support one, because otherwise, we might end up with these too fragmented approaches for doing the same thing again.

Yes. I agree with this. I've aligned fields and vectors with metadata, supporting both approaches. Working with dict is fine for me, and is clearest to users since in the doc they are described as dict. Same feeling with IDE warnings when using attributes. I would say to keep the dict-like access and raise an error when accessing using attributes (I think the default behavior is do-nothing)

A question here, should we do the same for responses and suggestions? Now, we support:

record.responses.label record.suggestions.text

which have the same IDE problems.

Ok. So we'll open a new PR for suggestions and responses, where we'll use a dictionary style access as well? Nice.

burtenshaw · 2024-06-17T08:06:25Z

argilla/src/argilla/records/_resource.py

+        return [Vector(name=name, values=value).api_model() for name, value in self.items()]
+
+    def to_dict(self) -> Dict[str, List[float]]:
+        return {key: value for key, value in self.items()}


 class RecordResponses(Iterable[Response]):


How will this class be used when users want to:

add a first response after retrieving records?

add subsequent responses after retrieving records?

Ideally, these should just be the same.

For me, both cases should be something like this:

record.responses.add(rg.Response(...)..) # or record.responses.add[rg.Response(...), rg.Response(...)])

davidberenstein1957

Left some comments.

argilla/src/argilla/records/_dataset_records.py

davidberenstein1957 · 2024-06-17T09:20:58Z

argilla/src/argilla/records/_resource.py

-    def to_dict(self) -> Dict[str, Union[str, None]]:
-        return self.__fields
+    def to_dict(self) -> dict:
+        return {key: value for key, value in self.items()}


Suggested change

return {key: value for key, value in self.items()}

return dict(self.items())

davidberenstein1957 · 2024-06-17T09:23:17Z

argilla/src/argilla/records/_resource.py

+    def __init__(self, metadata: Optional[Dict[str, MetadataValue]] = None) -> None:
+        super().__init__(metadata or {})
+
+    def __getattr__(self, item: str):


I think it is clearer to only support one, because otherwise, we might end up with these too fragmented approaches for doing the same thing again.

davidberenstein1957 · 2024-06-17T09:24:00Z

argilla/src/argilla/records/_resource.py

+        self[key] = value
+
+    def to_dict(self) -> dict:
+        return {key: value for key, value in self.items()}


Suggested change

return {key: value for key, value in self.items()}

return dict(self.items())

davidberenstein1957 · 2024-06-17T09:24:45Z

argilla/src/argilla/records/_resource.py

+        return [Vector(name=name, values=value).api_model() for name, value in self.items()]
+
+    def to_dict(self) -> Dict[str, List[float]]:
+        return {key: value for key, value in self.items()}


Suggested change

return {key: value for key, value in self.items()}

return dict(self.items())

davidberenstein1957 · 2024-06-17T09:26:16Z

argilla/tests/unit/export/test_record_export_import_compatibillity.py

-        vectors=[rg.Vector("text", [0, 0, 0])],
-        id=str(uuid.uuid4()),
+        vectors={"text": [0, 0, 0]},
+


Suggested change

argilla/tests/unit/test_resources/test_records.py

# Description This PR addresses [discussion](#5026 (comment)) from PR #5026 and removes attribute-like access for record fields, vectors and metadata. **Type of change** (Please delete options that are not relevant. Remember to title the PR according to the type of change) - [ ] New feature (non-breaking change which adds functionality) - [ ] Refactor (change restructuring the codebase without changing functionality) - [ ] Improvement (change adding some improvement to an existing functionality) **How Has This Been Tested** (Please describe the tests that you ran to verify your changes. And ideally, reference `tests`) - [ ] Test A - [ ] Test B **Checklist** - [ ] I added relevant documentation - [ ] I followed the style guidelines of this project - [ ] I did a self-review of my code - [ ] I made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I filled out [the contributor form](https://tally.so/r/n9XrxK) (see text above) - [ ] I have added relevant notes to the `CHANGELOG.md` file (See https://keepachangelog.com/)

frascuchon added 12 commits June 14, 2024 11:30

chore: Align metadata models generation to the rest of the Record con…

7c80290

…tainers (api_model method)

chore: Define alias FieldValue

1ea3110

refactor: Redefine RecordFields as a dict-like container

681d5d2

tests: Add more unit tests for record fields

92f2b3a

refactor: align RecordMetadata container by adding to_dict method

e4a0ce4

refactor: Define VectorValue alias

a94e618

chore: Define __all__ module members

b6eddeb

refactor: Align vectors container with fields and metadata

80e9622

chore: Rename old external_id to id

574bde4

tests: Add new tests for record vectors

1fa2160

chore: Review method signature

d6780fc

fix: Normalize ids when using to_dict methods

0991739

frascuchon requested review from burtenshaw and davidberenstein1957 June 14, 2024 11:27

frascuchon changed the title ~~Feat/updating all fields~~ [ENHANCEMENT] Argilla SDK: Updating all fields Jun 14, 2024

frascuchon added this to the v2.0.0 milestone Jun 14, 2024

burtenshaw reviewed Jun 17, 2024

View reviewed changes

davidberenstein1957 reviewed Jun 17, 2024

View reviewed changes

frascuchon added 2 commits June 17, 2024 14:29

chore: Type definition for suggestions

0b37367

refactor: Organize methods order and improve to_dict methods

dc99812

frascuchon requested review from burtenshaw and davidberenstein1957 June 17, 2024 12:34

frascuchon self-assigned this Jun 18, 2024

frascuchon changed the title ~~[ENHANCEMENT] Argilla SDK: Updating all fields~~ [ENHANCEMENT] Argilla SDK: Updating record fields, vectors and metadata Jun 18, 2024

frascuchon changed the title ~~[ENHANCEMENT] Argilla SDK: Updating record fields, vectors and metadata~~ [ENHANCEMENT] Argilla SDK: Updating record fields and vectors Jun 18, 2024

davidberenstein1957 approved these changes Jun 18, 2024

View reviewed changes

burtenshaw approved these changes Jun 18, 2024

View reviewed changes

frascuchon merged commit 6452993 into feat/updating-record Jun 18, 2024
13 checks passed

frascuchon deleted the feat/updating-all-fields branch June 18, 2024 10:17

frascuchon mentioned this pull request Jun 18, 2024

[ENHANCEMENT] argilla: Remove attribute-like access #5048

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENHANCEMENT] Argilla SDK: Updating record fields and vectors #5026

[ENHANCEMENT] Argilla SDK: Updating record fields and vectors #5026

frascuchon commented Jun 14, 2024 •

edited

Loading

burtenshaw commented Jun 17, 2024 •

edited

Loading

burtenshaw Jun 17, 2024

davidberenstein1957 Jun 17, 2024

frascuchon Jun 17, 2024

frascuchon Jun 17, 2024

burtenshaw Jun 18, 2024

burtenshaw Jun 17, 2024

frascuchon Jun 17, 2024

davidberenstein1957 left a comment

davidberenstein1957 Jun 17, 2024

davidberenstein1957 Jun 17, 2024

davidberenstein1957 Jun 17, 2024

davidberenstein1957 Jun 17, 2024

davidberenstein1957 Jun 17, 2024

	return {key: value for key, value in self.items()}
	return dict(self.items())

[ENHANCEMENT] Argilla SDK: Updating record fields and vectors #5026

[ENHANCEMENT] Argilla SDK: Updating record fields and vectors #5026

Conversation

frascuchon commented Jun 14, 2024 • edited Loading

Description

burtenshaw commented Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidberenstein1957 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frascuchon commented Jun 14, 2024 •

edited

Loading

burtenshaw commented Jun 17, 2024 •

edited

Loading