Skip to content

Conversation

@meminkula
Copy link

This Pull Request addresses the issue of unnecessary vocabulary mapping tensors (t2d and d2t) being required for Eagle3Speculator models that utilize the full verifier vocabulary (as detailed in Issue #200). The core change involves updating the init method to accept t2d and d2t as optional arguments. The buffer registration logic in init and the mapping application in the forward pass are now conditional on these tensors being present. This ensures that users running full-vocab Eagle3 models can cleanly omit these buffers, which reduces checkpoint size and simplifies the developer workflow by removing the requirement for redundant "no-op" tensors. I'm excited to contribute this fix to the project.

Note: that was my first contribute. I hope i can be helpfull.

@meminkula meminkula changed the title feat: Make vocab mapping optional in Eagle3Speculator Make vocab mapping optional in Eagle3Speculator Dec 4, 2025
@meminkula meminkula changed the title Make vocab mapping optional in Eagle3Speculator Make vocab mapping optional Dec 4, 2025
from typing import Any, ClassVar, Literal

import torch
from django.contrib.gis.gdal.prototypes.srs import from_user_input
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove this import if it's not used?

@fynnsu
Copy link
Collaborator

fynnsu commented Dec 16, 2025

Hi @meminkula, thank you for contributing. Since this is your first contribution, maybe I can give you some pointers to get you started:

  • The focus of Create Eagle3DraftModels which use the full verifier vocab #200 is on the Eagle3DraftModel which is located here. We're actually in the process of deprecating and removing the Eagle3Speculator model you modified, so no changes are needed for that.
  • When contributing to open source projects in general, it's important to keep the changes as minimal as possible. So changes like adding a django import or implementing the full forward function aren't necessary for this issue, and can distract from the focus of the pr.
  • Lastly, we have some additional requirements when contributing to this repo that require you follow code formatting and linting steps before submitting. There's more information on that here

Let me know if you have any questions.

@dsikka
Copy link
Collaborator

dsikka commented Jan 15, 2026

Hi @meminkula are you still interested in landing this PR?

@dsikka
Copy link
Collaborator

dsikka commented Jan 15, 2026

@Mergifyio refresh

@mergify
Copy link

mergify bot commented Jan 15, 2026

refresh

✅ Pull request refreshed

@mergify
Copy link

mergify bot commented Jan 15, 2026

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @meminkula.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Jan 15, 2026
@dsikka
Copy link
Collaborator

dsikka commented Jan 20, 2026

@meminkula are you still interested in landing this? If yes, can you resolve the merge conflicts?

@fynnsu
Copy link
Collaborator

fynnsu commented Jan 21, 2026

Closing this as stale. @meminkula if you would like keep working on this, please let us know / reopen the pr.

@fynnsu fynnsu closed this Jan 21, 2026
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.

4 participants