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

Fix idefics3 #133

Merged
merged 3 commits into from
Nov 28, 2024
Merged

Fix idefics3 #133

merged 3 commits into from
Nov 28, 2024

Conversation

Blaizzy
Copy link
Owner

@Blaizzy Blaizzy commented Nov 28, 2024

This PR addresses two critical bugs that were introduced in PR #124:

Changes Made

  • Modified token placement algorithm to insert image tokens at the beginning of the sequence instead of appending them at the end
  • Implemented proper index tracking and reporting for image tokens
  • Added validation to ensure token indices are correctly maintained

Visual Demonstration

Input Image

Sample menu section used for testing:
menu_section_1

Before ❌

Token placement and index reporting issues visible in these screenshots:
Screenshot 2024-11-28 at 3 57 18 PM
Screenshot 2024-11-28 at 3 57 36 PM

After ✅

Correct token placement and index reporting:
Screenshot 2024-11-28 at 3 57 50 PM
Screenshot 2024-11-28 at 3 58 00 PM

Testing

  • Verified token placement in multiple scenarios
  • Confirmed correct index reporting across different image types
  • Validated backwards compatibility with existing implementations

Related Issues

@andimarafioti
Copy link
Contributor

idefics3 and smolvlm have different language models and tokenizers! So you need to get this from the model

@andimarafioti
Copy link
Contributor

model.config.image_token_id for idefics3

@Blaizzy
Copy link
Owner Author

Blaizzy commented Nov 28, 2024

Thanks for pointing it out!

The image_token_id and all other arguments will be overriden when a model with different config but same arch is loaded.
image

@Blaizzy Blaizzy merged commit 331fbc6 into main Nov 28, 2024
1 check passed
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.

2 participants