Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: follow-up fixes for detached pydantic.BaseModel schemas #3829

Merged
merged 20 commits into from
Oct 11, 2023

Conversation

alvarobartt
Copy link
Member

@alvarobartt alvarobartt commented Sep 26, 2023

Description

This PR contains some bug-fixes and improvements as a follow up for #3784.

  • FeedbackDataset from the Hugging Face Hub fails if exported with an outdated version as it still contained the ID for some columns
  • Adding records via add_records over an existing FeedbackDataset in Argilla i.e. a RemoteFeedbackDataset fails because the _fields_schema is not defined
  • Shared attribute initialization in FeedbackDatasetBase, but move local validation to FeedbackDataset
  • Extend supported type-hints in generate_pydantic_schema function
  • Fix from_huggingface to be backwards compatible with previously uploaded FeedbackDataset datasets to the Hugging Face Hub (from Argilla v1.8.0)

Kudos @frascuchon for detecting and reporting the bugs tackled in this PR!

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (change adding some improvement to an existing functionality)

How Has This Been Tested

  • Run rg.FeedbackDataset.from_huggingface("argilla/oasst_response_quality", split="train")
  • Add outdated/deprecated configuration files to check that DatasetConfig.from_yaml works as intended
  • Add integration tests to ensure that adding a record to an existing FeedbackDataset in Argilla works as expected

Checklist

  • I added relevant documentation
  • follows 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/)

To avoid using the schema defined in the SDK

Co-authored-by: Paco Aranda <[email protected]>
@alvarobartt alvarobartt marked this pull request as ready for review October 11, 2023 08:41
@alvarobartt alvarobartt self-assigned this Oct 11, 2023
@alvarobartt alvarobartt merged commit 7243b71 into develop Oct 11, 2023
14 of 16 checks passed
@alvarobartt alvarobartt deleted the fix/after-schema-refactor branch October 11, 2023 14:11
alvarobartt added a commit that referenced this pull request Oct 13, 2023
I believe this patch was done before due to an incompatibility right after #3784 which is already patched at #3829, so this is not required now, also because it's unsafe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants