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 PrefixSuffix multiseq mappers to prepend/append tokens #12

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

MaksymDel
Copy link
Contributor

@MaksymDel MaksymDel commented Aug 2, 2022

Hi! With the introduction of Smashed, munging datasets of long documents is going to be a lot more fun )

This draft PR is simply to explore the idea below. It does the following:

  1. It adds a new CustomTokensSequencePaddingMapper with corresponding classes for type_ids and attnetion_mask that allow wrapping the sentences with custom ids or strings.
  2. It abstracts away the SequencePaddingMapper to do the general job of adding prefix/suffix tokens depending on the sentence number

In (1), we might want to prepend strings because Text2Text models like T5 expect inputs to have Task prefixes.
And we might want to bound sentences with custom special token_ids (e.g., with tokenizer added special tokens) to indicate the type of sentences in the dataset column.

Because of (2), subclasses now do not have to implement the transform function and only define what prefix/suffix tokens to add.

On the pro side, it reduces code duplication (especially considering new CustomPadding classes) and unifies the classes.
But on the con side, we now have one more level of inheritance...

If some variation of this proposal fits, I can add docs and tests.

@soldni

@soldni
Copy link
Member

soldni commented Aug 3, 2022

Hi Maksym!

Thank you for this pull request. I fully support the reasoning behind adding these mappers, but I would prefer avoiding Mappers argument that are callables, as they can create issues with libraries that use function signatures (I have another PR to remove that from MakeFieldMapper, actually).

In general, I would rather have more specialized mappers that do one thing well than fewer, more generic mappers that are highly configurable. It does make for more legible code, and it is easier to maintain.

@soldni soldni marked this pull request as ready for review August 5, 2022 00:40
@soldni soldni marked this pull request as draft August 5, 2022 00:40
@MaksymDel MaksymDel marked this pull request as ready for review August 5, 2022 01:36
@MaksymDel MaksymDel changed the title Add Mappers for arbitrary tokens and abstract away SequencePaddingMapper; Add PrefixSuffix multiseq mappers to prepend/append tokens Aug 5, 2022
@MaksymDel
Copy link
Contributor Author

MaksymDel commented Aug 5, 2022

Hi Luca!

I removed callable arguments and added tests.

I think this is ready for review.

@MaksymDel
Copy link
Contributor Author

Hi @soldni!

Does the current state of this PR look good, or is it better to redirect the efforts elsewhere at the moment?

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.

None yet

2 participants