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/hf datasets summarization default prompt #623

Merged
merged 6 commits into from
Jul 15, 2023

Conversation

Prikshit7766
Copy link
Contributor

Checklist:

  • I've added Google style docstrings to my code.
  • I've used pydantic for typing when/where necessary.
  • I have linted my code
  • I have added tests to cover my changes.

@Prikshit7766 Prikshit7766 added the 🐛 Bug Something isn't working label Jul 14, 2023
Comment on lines 680 to 683
dataset_name = (
self.dataset_name.split("-")[0].lower() if self.dataset_name else "sum_prompt"
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Does that refer to one of the key in default_user_prompt?? If yes, let's rename the key directly so that we don't have to handle edge cases

Copy link
Contributor Author

@Prikshit7766 Prikshit7766 Jul 14, 2023

Choose a reason for hiding this comment

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

@JulesBelveze
right know we were acessing the default_user_prompt based on dataset that we have in langtest

But, huggingface contain so many dataset so we will be using sum_prompt for all the huggingface dataset where task is summarization.

This is not the bug , this is is already in pr - #589

but due to some conflict I think the changes are not updated in the release/1.1.0 branch

Copy link
Contributor

@JulesBelveze JulesBelveze Jul 14, 2023

Choose a reason for hiding this comment

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

@Prikshit7766 Gotcha, could we rename it to something more self explanatory like "hf-dataset" or something? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JulesBelveze
This prompt is like default summarization prompt not restricted to hf-dataset only.
if you have any suggestion we can do that

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's go for default-summarization-prompt

@JulesBelveze
Copy link
Contributor

@Prikshit7766 In general it is good practice to either describe the bug you are trying to fix or link it to a GH issue. So that we know what you are trying to fix and can keep track of old bugs

@JulesBelveze JulesBelveze merged commit 6990ba1 into release/1.1.0 Jul 15, 2023
@ArshaanNazir ArshaanNazir deleted the fix/hf-datasets-summarization branch July 18, 2023 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants