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: unhandled ImportError on huggingface_hub #3174

Merged
merged 2 commits into from
Jun 14, 2023

Conversation

alvarobartt
Copy link
Member

@alvarobartt alvarobartt commented Jun 13, 2023

Description

As reported by @dvsrepo, when installing argilla without any extra dependency, an unhandled ImportError is raised. That's due to the import of ArgillaDatasetCard being on top of the file instead of under the @requires_version decorator, so that the ImportError will still be raised, but at least it will be handled.

Also the ImportError was being triggered just when importing the FeedbackDataset class as it was on top of the file, which was obviously wrong, as it should just raise when calling push_to_huggingface.

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)

  • Re-run unit tests locally on Python 3.11
  • Make sure that from argilla.client.feedback.dataset import FeedbackDataset doesn't raise an ImportError when huggingface_hub is not installed

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/)

@alvarobartt alvarobartt added this to the v1.10.0 milestone Jun 13, 2023
@alvarobartt alvarobartt requested a review from dvsrepo June 13, 2023 18:12
@davidberenstein1957
Copy link
Member

_ can review this tomorrow_

@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.26 🎉

Comparison is base (292d2e3) 90.64% compared to head (d0cd7d9) 90.91%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3174      +/-   ##
===========================================
+ Coverage    90.64%   90.91%   +0.26%     
===========================================
  Files          215      215              
  Lines        11312    11315       +3     
===========================================
+ Hits         10254    10287      +33     
+ Misses        1058     1028      -30     
Flag Coverage Δ
pytest 90.91% <100.00%> (+0.26%) ⬆️

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

Impacted Files Coverage Δ
src/argilla/client/feedback/dataset.py 82.96% <100.00%> (ø)
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%) ⬆️

... and 3 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.

@alvarobartt alvarobartt merged commit bfd4057 into develop Jun 14, 2023
@alvarobartt alvarobartt deleted the fix/unhandled-import-error branch June 14, 2023 10:26
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