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/3347 feature add unification support for the rankingquestion #3364

Conversation

davidberenstein1957
Copy link
Member

@davidberenstein1957 davidberenstein1957 commented Jul 9, 2023

Description

  • renamed typing.py to types.py to avoid import errors
  • added RankingQuestionStrategy
  • added RankingQuestionUnification
  • added RankingQuestionsupport for the .for_text_classification method for the TrainingTaskMapping

Closes #3347

Type of change

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

  • New feature (non-breaking change which adds functionality)
  • Improvement (change adding some improvement to an existing functionality)

How Has This Been Tested

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

  • tests/client/feedback/test_schemas.py:test_ranking_question_strategy
  • tests/client/feedback/training/test_schemas.py:test_task_mapping_for_text_classification

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

@davidberenstein1957 davidberenstein1957 marked this pull request as ready for review July 9, 2023 14:21
CHANGELOG.md Outdated Show resolved Hide resolved
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.

Overall LGTM 👍🏻 Just some minor comments and some things to clarify in order to have more context!

@dvsrepo
Copy link
Member

dvsrepo commented Jul 10, 2023

Hi! As I have limited bandwidth but this is a key feature for Reward Modeling + RLHF. Could someone add an example (with code snippets) to see what's the expected output of this prepare for training method?

Specifically I'm interested and want to double check if we're making it easy or targeting to prepare data for Reward Modeling, i.e., creating pairs of chosen rejected responses (as we've shown with this tutorial. If this is not tackled yet, we need to prepare a brief spec for supporting it.

@davidberenstein1957
Copy link
Member Author

davidberenstein1957 commented Jul 10, 2023 via email

davidberenstein1957 and others added 2 commits July 11, 2023 08:24
Co-authored-by: Alvaro Bartolome <alvaro@argilla.io>
chore: updated changelog
chore: added docstrings
@davidberenstein1957
Copy link
Member Author

davidberenstein1957 commented Jul 11, 2023

@dvsrepo
Reward Modelling
trl #3377
trlx#3324
SFT/promp-completion
trl #3379
trlx #3378

@davidberenstein1957
Copy link
Member Author

@tomaarsen I also made some changes here that might be relevant, but I saw this was not merged yet.

@codecov
Copy link

codecov bot commented Jul 16, 2023

Codecov Report

Patch coverage: 93.60% and project coverage change: +0.31 🎉

Comparison is base (6630d7b) 90.13% compared to head (03c6dd9) 90.45%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3364      +/-   ##
===========================================
+ Coverage    90.13%   90.45%   +0.31%     
===========================================
  Files          233      243      +10     
  Lines        12493    13223     +730     
===========================================
+ Hits         11261    11961     +700     
- Misses        1232     1262      +30     
Flag Coverage Δ
pytest ?

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

Impacted Files Coverage Δ
...ack/integrations/huggingface/card/_dataset_card.py 100.00% <ø> (ø)
.../feedback/integrations/huggingface/card/_parser.py 100.00% <ø> (ø)
src/argilla/client/feedback/types.py 100.00% <ø> (ø)
src/argilla/client/sdk/commons/errors.py 72.22% <ø> (ø)
src/argilla/feedback/__init__.py 100.00% <ø> (ø)
src/argilla/tasks/database/migrate.py 39.13% <ø> (-4.87%) ⬇️
src/argilla/training/autotrain_advanced.py 0.00% <0.00%> (ø)
src/argilla/utils/telemetry.py 89.09% <ø> (ø)
src/argilla/client/feedback/training/schemas.py 87.50% <50.00%> (-1.01%) ⬇️
src/argilla/server/settings.py 77.41% <50.00%> (-3.76%) ⬇️
... and 72 more

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

@davidberenstein1957 davidberenstein1957 merged commit 0be77e3 into develop Jul 16, 2023
@davidberenstein1957 davidberenstein1957 deleted the feat/3347-feature-add-unification-support-for-the-rankingquestion branch July 16, 2023 06:20
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.

[FEATURE] add unification support for the RankingQuestion
3 participants