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

Apply proper serializer when pushing feedback dataset in Argilla #3192

Conversation

frascuchon
Copy link
Member

@frascuchon frascuchon commented Jun 14, 2023

Description

By default, the model.dict() method won't apply custom serialization to the UUID or datetatime fields. Since pydantic provides a .json method which applies serialization properly, we should use them in order to support pushing data in Argilla.

The current solution may be improved since we're serializing more times than needed. But for that, we should change the sdk.v1.api layer. See issue #3191

Closes #3189

Type of change

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

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested

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

New tests have been added

Checklist

  • I have merged the original branch into my forked branch
  • 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 have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

Copy link
Member

@alvarobartt alvarobartt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just some minor comments regarding the json.loads(schema.json()), and about the removal of .dict(includes=set()) 🤗

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.22 ⚠️

Comparison is base (51751ac) 90.91% compared to head (046c1ba) 90.69%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3192      +/-   ##
===========================================
- Coverage    90.91%   90.69%   -0.22%     
===========================================
  Files          215      215              
  Lines        11304    11318      +14     
===========================================
- Hits         10277    10265      -12     
- Misses        1027     1053      +26     
Flag Coverage Δ
pytest 90.69% <100.00%> (-0.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/argilla/__init__.py 83.33% <ø> (ø)
src/argilla/_version.py 100.00% <100.00%> (ø)
src/argilla/client/apis/datasets.py 90.37% <100.00%> (ø)
src/argilla/client/feedback/dataset.py 83.69% <100.00%> (+1.17%) ⬆️
src/argilla/client/feedback/schemas.py 98.21% <100.00%> (ø)
src/argilla/client/sdk/v1/datasets/api.py 89.00% <100.00%> (+0.11%) ⬆️
src/argilla/server/apis/v1/handlers/datasets.py 100.00% <100.00%> (ø)
src/argilla/server/schemas/v1/datasets.py 100.00% <100.00%> (ø)
src/argilla/server/search_engine.py 92.24% <100.00%> (+0.12%) ⬆️
src/argilla/training/span_marker.py 97.77% <100.00%> (ø)

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Alvaro Bartolome <alvaro@argilla.io>
@frascuchon frascuchon merged commit e33a2df into develop Jun 14, 2023
@frascuchon frascuchon deleted the 3189-bug-cannot-copy-feedback-dataset-to-be-published-as-another-nameworkspace branch June 14, 2023 16:27
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.

[BUG] Cannot copy Feedback dataset to be published as another name/workspace
2 participants