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.

Sign up for GitHub

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

Jump to bottom

[UDOP] Add special tokens to tokenizer #29594

Merged
merged 10 commits into from
Apr 19, 2024
Merged

[UDOP] Add special tokens to tokenizer #29594

merged 10 commits into from
Apr 19, 2024
+43 −3

Conversation

NielsRogge
Copy link
Contributor

What does this PR do?

This PR makes sure the 1201 additional special tokens can be encoded/decoded properly.

Fixes #29591

@NielsRogge
Add special tokens

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
af6b301
@NielsRogge
Add special tokens
f4fb474
@NielsRogge
Use fmt
Loading
Loading status checks…
5a15a02
@NielsRogge
Uncomment code
Loading
Loading status checks…
6df7ee5
@NielsRogge
Add test
a036a6e
@NielsRogge
Remove scripts
3543e20
@HuggingFaceDocBuilderDev
Copy link

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

ArthurZucker
ArthurZucker reviewed Mar 17, 2024
View reviewed changes
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

LGTM, let's do better testing:
paragraph<loc_58>. Hey for example with a special case

@NielsRogge
Copy link
Contributor Author

Thanks, I've added tests. Should we add spaces_between_special_tokens as argument to UdopTokenizer, to make it decode the same as the fast tokenizer?

@ArthurZucker
Copy link
Collaborator

You can add it and default it to False in the decode maybe? Cuz I don't think decode takes self. spaces_between_special_tokens into account no?

@NielsRogge
Copy link
Contributor Author

Yeah indeed, shouldn't the _decode method here take into account self. spaces_between_special_tokens in case it exists?

@NielsRogge NielsRogge requested a review from ArthurZucker April 17, 2024 06:32
ArthurZucker
ArthurZucker approved these changes Apr 17, 2024
View reviewed changes
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

LGTM, breaking but fixing

@NielsRogge
Copy link
Contributor Author

NielsRogge commented Apr 17, 2024 •
edited
Loading

I don't think it's a breaking change, since adding spaces_between_special_tokens as an attribute doesn't do anything. The _decode method doesn't use it (whereas I think it should).

@ArthurZucker
Copy link
Collaborator

If it does not do anything why are we adding it?

@NielsRogge
Copy link
Contributor Author

Haha yes I'll remove it, and I'll remove it for Gemma too

@NielsRogge NielsRogge merged commit ecfe9be into huggingface:main Apr 19, 2024
19 checks passed
ydshieh pushed a commit that referenced this pull request Apr 23, 2024
@NielsRogge @ydshieh
[UDOP] Add special tokens to tokenizer (#29594)
968cda2 
* Add special tokens

* Add special tokens

* Use fmt

* Uncomment code

* Add test

* Remove scripts

* Address comments

* Improve tests

* Address comment

* Remove flag
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArthurZucker ArthurZucker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Special tokens of UDOP aren't encoded/decoded correctly
3 participants
@NielsRogge @HuggingFaceDocBuilderDev @ArthurZucker