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

Add new setting: prompt_column_separator #724

Merged
merged 15 commits into from
Jul 10, 2024
Merged

Add new setting: prompt_column_separator #724

merged 15 commits into from
Jul 10, 2024

Conversation

pascal-pfeiffer
Copy link
Collaborator

closes #483

@pascal-pfeiffer pascal-pfeiffer marked this pull request as draft May 24, 2024 12:09
@pascal-pfeiffer pascal-pfeiffer marked this pull request as ready for review June 25, 2024 10:11
@sherenem
Copy link
Collaborator

Docs LGTM, thank you!

Copy link
Collaborator

@psinger psinger left a comment

Choose a reason for hiding this comment

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

THX - overall looks good!

A little feedback:

  • Shall we add a different default separator, like \n###\n or similar?
  • I guess it is not possible to add a nesting that only triggers if multiple values are selected? Possible probably, but some work, not sure if worth.
  • Otherwise I would suggest to add it to the tokenizer section though. It is a bit dominant on top.

@@ -40,6 +40,7 @@ class ConfigNLPCausalLMDataset(DefaultConfig):

system_column: str = "system"
prompt_column: Tuple[str, ...] = ("instruction", "input")
prompt_column_separator: str = ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be an empty string by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was double space before, which is hard to see in the textbox and also a bit unusual. Decided to change it to double newlines now. Will push the changes in a bit. Wdyt?

@pascal-pfeiffer
Copy link
Collaborator Author

  • Shall we add a different default separator, like \n###\n or similar?

After some thought, I decided to use \n\n, will push the changes in a bit.

  • I guess it is not possible to add a nesting that only triggers if multiple values are selected? Possible probably, but some work, not sure if worth.

I also thought about it, but couldn't find an easy way. It would also force us to use a trigger for the columns dropdown.

  • Otherwise I would suggest to add it to the tokenizer section though. It is a bit dominant on top.

From a logical standpoint, I believe it should be a dataset setting.

@pascal-pfeiffer pascal-pfeiffer merged commit 98854a1 into main Jul 10, 2024
4 checks passed
@pascal-pfeiffer pascal-pfeiffer deleted the pp/separator branch July 10, 2024 14:50
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] Text separator setting
3 participants