-
Notifications
You must be signed in to change notification settings - Fork 11
Update speculator config & converter to support hidden states indexing #142
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
Conversation
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
|
📦 Build Artifacts Available |
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
…culators into hz-update-config
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good, but now since we are removing the forward pass through the model, does it still make sense to keep the --validate/ --validate-device arguments?
Signed-off-by: shanjiaz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a couple comments
Signed-off-by: shanjiaz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few questions/nits which can be addressed in a follow up, good work on this, LGTM once we raise the NotImplementedError for forward passes
Signed-off-by: shanjiaz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have test cases for multiple decoder layers?
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question below which might require a fix.
Signed-off-by: shanjiaz <[email protected]>
|
LGTM pending quality! |
Signed-off-by: shanjiaz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Added tests and review has been addressed.
Changes:
eagle_aux_hidden_state_layer_idsandinference_type.target_vocab_size. We default on using "t2d" length, if not available, load the config file of verifier model, recursively search the dict forvocab_size. (The search is needed for nested dict. e.g. target_config_dict["text_config"]["vocab_size"] )Command used:
Converted checkpoint:
shanjiaz/Llama4-Maverick-Eagle3-Speculators-converted