fix: format_as("datasets") when no responses
#3224
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dvsrepo
reviewed
Jun 19, 2023
There was a problem hiding this comment.
cool @alvarobartt ! please ask someone to review this as I might just approve it blindly 😄
frascuchon
reviewed
Jun 20, 2023
frascuchon
reviewed
Jun 20, 2023
frascuchon
reviewed
Jun 20, 2023
# Description Due to the recent constraint for the `FeedbackRecord.user_id`s to be `UUID`s instead of `str`s, the conversion to `dict()` was failing, as the `UUID`s are not JSON-serializable, which was leading to some issues when trying to add the example record to the `DatasetCard` via the `dict()` conversion. **Type of change** - [X] Bug fix (non-breaking change which fixes an issue) **How Has This Been Tested** - [X] Re-run unit tests to pass when generating the `DatasetCard` for `FeedbackDataset`s **Checklist** - [X] I have merged the original branch into my forked branch - [ ] I added relevant documentation - [X] follows the style guidelines of this project - [X] I did a self-review of my code - [ ] I made corresponding changes to the documentation - [X] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #3224 +/- ##
===========================================
+ Coverage 90.91% 91.03% +0.12%
===========================================
Files 215 218 +3
Lines 11304 11512 +208
===========================================
+ Hits 10277 10480 +203
- Misses 1027 1032 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
gabrielmbmb
approved these changes
Jun 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
As just reported by @dvsrepo, the following flow:
FeedbackDataset.from_argilla() -> .format_as("datasets")was failing under some scenarios where either the optional responses had no value or there were no responses at all, as either we were trying to retrieve those when any, or we were setting it's value toNoneif not found, instead of{"user_id": None, "values": None, "status": None}which is what 🤗 Datasets is producing based on the features.So on, this PR solves that bug and now the flow mentioned above is possible and works as expected!
Type of change
How Has This Been Tested
feedback_dataset_recordshas been modified to contain a record with and another without responsesChecklist