Skip to content

Fix Phi-3.5-vision eos token temporary fix#1804

Open
dhandhalyabhavik wants to merge 1 commit into
huggingface:mainfrom
dhandhalyabhavik:phi-3-fix
Open

Fix Phi-3.5-vision eos token temporary fix#1804
dhandhalyabhavik wants to merge 1 commit into
huggingface:mainfrom
dhandhalyabhavik:phi-3-fix

Conversation

@dhandhalyabhavik

Copy link
Copy Markdown

What does this PR do?

As pointed our correctly in issue #1630 with default config provided by microsoft/Phi-3.5-vision-instruct results into infinite generation loop. The issue is mismatch in stale eos_token_id=2 in generation_config.json while tokenizer's actual eos token is 32000. This fix modifies eos_token_id to be [2, 32000]. I have added #TODO to remove it in future when upstream config gets fixed.

Fixes # (issue)
#1630

Before submitting

  • [NA] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • [NA] Did you make sure to update the documentation with your changes?
  • [NA] Did you write any new necessary tests?

generation_config = getattr(model, "generation_config", None)
if generation_config is not None:
try:
# Todo: delete this check once phi-3 eos token get fixed in generation_configs.json

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can you please provide discussion raised in Github or HF about this issue? And refer with the link.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I found following discussion on HF,

https://huggingface.co/microsoft/Phi-3.5-vision-instruct/discussions/40

No discussion on GitHub transformers repo.

The script sample_inference.py on HF official repo replaces EOS token at generation, checkout this line - https://huggingface.co/microsoft/Phi-3.5-vision-instruct/blob/main/sample_inference.py#L58

@rkazants rkazants left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

please implement test

@dhandhalyabhavik

Copy link
Copy Markdown
Author

please implement test

@rkazants Done, added EOS specific tests with issue number. If upstream HF gets updated then we can later on remove the temp fix. Rebased the repo to inherit latest changes.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants