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

feat: add tqdm bar in FeedbackDataset.push_to_argilla #3233

Merged

Conversation

manulpatel
Copy link
Contributor

@manulpatel manulpatel commented Jun 20, 2023

Description

Add a tqdm progress bar to FeedbackDataset.push_to_argilla when looping over the records to upload, either if those are all the records in records or just the latest/new records in __new_records.

Closes #3119

Type of change

  • Improvement (change adding some improvement to an existing functionality)

How Has This Been Tested
Manully ran a test.py to push a dataset to argilla and checked the appreance of tqdm bar in the terminal while adding records.

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 missing the description! 👍🏻

src/argilla/client/feedback/dataset.py Outdated Show resolved Hide resolved
@alvarobartt
Copy link
Member

Hi again @manulpatel thanks again for the great work! I think we can merge this PR right after #3234 since we're also fixing some bugs there, would you mind updating the CHANGELOG.md with a brief description of your changes an the URL pointing to this PR? Thanks!

@manulpatel
Copy link
Contributor Author

Sure! Np! I need to add my changes under 1.11.0 in CHANGELOG.md in the same PR, correct?

@alvarobartt
Copy link
Member

Sure! Np! I need to add my changes under 1.11.0 in CHANGELOG.md in the same PR, correct?

Right! Under 1.11.0 as we'll be releasing it tomorrow!

Copy link
Member

@gabrielmbmb gabrielmbmb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @manulpatel! I've left some comments :)

@@ -581,7 +586,7 @@ def delete_dataset(dataset_id: UUID) -> None:
datasets_api_v1.add_records(
client=httpx_client,
id=argilla_id,
records=[json.loads(record.json()) for record in batch],
records=[json.loads(record.dict()) for record in tqdm(batch)],
Copy link
Member

Choose a reason for hiding this comment

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

This won't work! json.loads expects either a str or bytes. Can you revert this change?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed that's already modified in the #3234 branch as mentioned below! But yes, that would run into conflicts once the other PR is merged into develop

src/argilla/client/feedback/dataset.py Show resolved Hide resolved
@alvarobartt
Copy link
Member

Also @manulpatel make sure to install the pre-commit to avoid the PR from failing due to that 😄 To do so you just need to install pip install pre-commit and from the root directory of the project run pre-commit install, then before every commit the pre-commit will run and fix the issues if applicable!

@manulpatel
Copy link
Contributor Author

manulpatel commented Jun 21, 2023

@alvarobartt So sorry. Actually I have installed the pre-commit hooks locally but the above commit I did directly from github editor, which didn't run the . In the next commits I will ensure this strictly.

@alvarobartt
Copy link
Member

@alvarobartt So sorry. Actually I have installed the pre-commit hooks locally but the above commit I did directly from github editor, which didn't run the . In the next commits I will ensure this strictly.

No worries at all @manulpatel 😄

CHANGELOG.md Outdated Show resolved Hide resolved
src/argilla/client/feedback/dataset.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (d5632a3) 91.00% compared to head (b51db3c) 91.00%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3233   +/-   ##
========================================
  Coverage    91.00%   91.00%           
========================================
  Files          219      219           
  Lines        11672    11672           
========================================
  Hits         10622    10622           
  Misses        1050     1050           
Flag Coverage Δ
pytest 91.00% <100.00%> (ø)

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

Impacted Files Coverage Δ
src/argilla/client/feedback/dataset.py 83.68% <100.00%> (ø)

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

@gabrielmbmb gabrielmbmb merged commit 2c81f61 into argilla-io:develop Jun 21, 2023
damianpumar pushed a commit that referenced this pull request Jun 27, 2023
<!-- Thanks for your contribution! As part of our Community Growers
initiative 🌱, we're donating Justdiggit bunds in your name to reforest
sub-Saharan Africa. To claim your Community Growers certificate, please
contact David Berenstein in our Slack community or fill in this form
https://tally.so/r/n9XrxK once your PR has been merged. -->

# Description

Add a `tqdm ` progress bar to `FeedbackDataset.push_to_argilla` when
looping over the records to upload, either if those are all the records
in records or just the latest/new records in __new_records.

Closes #3119 

**Type of change**
- [x] Improvement (change adding some improvement to an existing
functionality)

**How Has This Been Tested**
Manully ran a test.py to push a dataset to argilla and checked the
appreance of `tqdm `bar in the terminal while adding records.

**Checklist**

- [ ] 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
- [x] I have added relevant notes to the CHANGELOG.md file (See
https://keepachangelog.com/)

---------

Co-authored-by: Alvaro Bartolome <alvarobartt@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

Add a tqdm bar in FeedbackDataset.push_to_argilla
3 participants