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

change sequence_bias type of SequenceBiasLogitsProcessor to list, add… #33375

Merged
merged 5 commits into from
Sep 19, 2024
Merged

change sequence_bias type of SequenceBiasLogitsProcessor to list, add… #33375

merged 5 commits into from
Sep 19, 2024

Conversation

VladOS95-cyber
Copy link
Contributor

@VladOS95-cyber VladOS95-cyber commented Sep 8, 2024 •
edited
Loading

What does this PR do?

Introduce new sequence_bias parameter type as List[List[List[int], float]] for SequenceBiasLogitsProcessor and config tests for all logits processors

Fixes # (issue)

Before submitting

Who can review?

@gante @zucchini-nlp @ArthurZucker
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@VladOS95-cyber
Copy link
Contributor Author

VladOS95-cyber commented Sep 8, 2024 •
edited
Loading

Hi @gante, please, review this PR. I completely messed with previous one #33362. This is cleaner, I resolved all your comments from the last time and included config tests for almost all processors

@VladOS95-cyber VladOS95-cyber changed the title change sequence_bias type of SequenceBiasLogitsProcessor tp list, add… change sequence_bias type of SequenceBiasLogitsProcessor type to list, add… Sep 9, 2024
@VladOS95-cyber VladOS95-cyber changed the title change sequence_bias type of SequenceBiasLogitsProcessor type to list, add… change sequence_bias type of SequenceBiasLogitsProcessor to list, add… Sep 9, 2024
@VladOS95-cyber VladOS95-cyber force-pushed the new_sequence_bias_type_for_SequenceBiasLogitsProcessor_config_tests branch from 0a8bf7b to 2754485 Compare September 10, 2024 16:20
@VladOS95-cyber
Copy link
Contributor Author

Just add @zucchini-nlp and @ArthurZucker in case if @gante is busy

@VladOS95-cyber
Copy link
Contributor Author

@zucchini-nlp Hi! Do I have to fix failed tests somehow? Because it looks like it is not related to my changes at all

@zucchini-nlp
Copy link
Member

Failing tests are okey, they are also failing in other PRs. You can try rebasing main to see if they are already fixed, the last time it was still broken in one of my PRs

@VladOS95-cyber
Copy link
Contributor Author

Failing tests are okey, they are also failing in other PRs. You can try rebasing main to see if they are already fixed, the last time it was still broken in one of my PRs

Ok, got it, thanks a lot!

@VladOS95-cyber
Copy link
Contributor Author

Hi @gante! Just a kindly reminder ping in order to not loose this PR!

gante
gante approved these changes Sep 16, 2024
View reviewed changes
Copy link
Member

@gante gante left a comment

Choose a reason for hiding this comment

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

Thank you for iterating @VladOS95-cyber -- and especially for adding the new tests. I really appreciate the work 🙏

IMO the PR could be merged as is. I've added a few improvement suggestions for long-term maintenance.

Regarding red CI -- try rebasing the PR. If it's still red after that, I will dive to figure out what's wrong :)

src/transformers/generation/logits_process.py Outdated Show resolved Hide resolved
src/transformers/generation/logits_process.py Outdated Show resolved Hide resolved
src/transformers/generation/logits_process.py Outdated Show resolved Hide resolved
tests/generation/test_configuration_utils.py Outdated Show resolved Hide resolved
tests/generation/test_configuration_utils.py Show resolved Hide resolved
@gante
Copy link
Member

gante commented Sep 16, 2024

@LysandreJik some context :D

this PR:

  1. Changes the type of the sequence_bias. The previous type was not json serializable. We keep BC, but all documentation points towards the serializable type (so that users don't hit the limitation)
  2. @VladOS95-cyber kindly added tests for serialization of logits processors arguments in the generation config 💛

@VladOS95-cyber VladOS95-cyber force-pushed the new_sequence_bias_type_for_SequenceBiasLogitsProcessor_config_tests branch from 2754485 to 2590fc4 Compare September 16, 2024 15:14
@VladOS95-cyber
Copy link
Contributor Author

VladOS95-cyber commented Sep 16, 2024 •
edited
Loading

Thank you for iterating @VladOS95-cyber -- and especially for adding the new tests. I really appreciate the work 🙏

IMO the PR could be merged as is. I've added a few improvement suggestions for long-term maintenance.

Regarding red CI -- try rebasing the PR. If it's still red after that, I will dive to figure out what's wrong :)

Hi @gante, I just pushed changes based on your comments and did rebase, still the same test issues

@gante
Copy link
Member

gante commented Sep 17, 2024

@VladOS95-cyber I've checked internally -- it is a known issue and it is being fixed :)

By the time @LysandreJik approves the PR it should be fixed

LysandreJik
LysandreJik approved these changes Sep 19, 2024
View reviewed changes
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

That's a very impressive additional test suite!

The change looks good to me, thanks for updating the docs alongside it.

@VladOS95-cyber
Copy link
Contributor Author

VladOS95-cyber commented Sep 19, 2024 •
edited
Loading

That's a very impressive additional test suite!

The change looks good to me, thanks for updating the docs alongside it.

Hi @LysandreJik sure, no problem! Should I do smth else, like rebase to re-run failed tests or anything?

@gante
Copy link
Member

gante commented Sep 19, 2024

Hey @VladOS95-cyber -- rebasing should get us rid of the red CI!

@VladOS95-cyber
Copy link
Contributor Author

Hey @VladOS95-cyber -- rebasing should get us rid of the red CI!

Hey @gante, ok, no worries, I'll do that

@VladOS95-cyber VladOS95-cyber force-pushed the new_sequence_bias_type_for_SequenceBiasLogitsProcessor_config_tests branch from 2590fc4 to 956ab38 Compare September 19, 2024 16:22
@gante
Copy link
Member

gante commented Sep 19, 2024

green CI, perfect!

@VladOS95-cyber thank you for iterating with us and, more importantly, for making transformers better for everyone 💛

@gante gante merged commit 162056a into huggingface:main Sep 19, 2024
20 checks passed
@VladOS95-cyber
Copy link
Contributor Author

green CI, perfect!

@VladOS95-cyber thank you for iterating with us and, more importantly, for making transformers better for everyone 💛

@gante Thank you too! Let's keep working on it!

itazap pushed a commit to NielsRogge/transformers that referenced this pull request Sep 20, 2024
@VladOS95-cyber @itazap
change sequence_bias type of SequenceBiasLogitsProcessor to list, add… (
2e93fae 
huggingface#33375)

* change sequence_bias type of SequenceBiasLogitsProcessor tp list, add config tests for all processors

* fix format

* small fix for all_token_bias_pairs_are_valid internal func

* small typo fix in description

* improve test impl, some SequenceBiasLogitsProcessor refactoring
amyeroberts pushed a commit to amyeroberts/transformers that referenced this pull request Oct 2, 2024
@VladOS95-cyber @amyeroberts
change sequence_bias type of SequenceBiasLogitsProcessor to list, add… (
a6e64a2 
huggingface#33375)

* change sequence_bias type of SequenceBiasLogitsProcessor tp list, add config tests for all processors

* fix format

* small fix for all_token_bias_pairs_are_valid internal func

* small typo fix in description

* improve test impl, some SequenceBiasLogitsProcessor refactoring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zucchini-nlp zucchini-nlp zucchini-nlp left review comments

@gante gante gante approved these changes

@LysandreJik LysandreJik LysandreJik approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@VladOS95-cyber @zucchini-nlp @gante @LysandreJik