Skip to content

Fix/hf datasets summarization default prompt#623

Merged
JulesBelveze merged 6 commits intorelease/1.1.0from
fix/hf-datasets-summarization
Jul 15, 2023
Merged

Fix/hf datasets summarization default prompt#623
JulesBelveze merged 6 commits intorelease/1.1.0from
fix/hf-datasets-summarization

Conversation

@Prikshit7766
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

2 participants