Skip to content

Conversation

@liangjason87
Copy link

No description provided.

Copy link
Member

@mmoskal mmoskal left a comment

Choose a reason for hiding this comment

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

Looks good! Now on to the rust part?

// these are only output tokens
uint32_t num_tokens;
int32_t const* tokens;
int32_t const* tokens; //c_resp.tokens = data.tokens.data();
Copy link
Member

Choose a reason for hiding this comment

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

not needed?

}

// If we have draft params build draft config
if (request->draft_params.draft_tokens && request->draft_params.logits_tensor.data_ptr)
Copy link
Member

Choose a reason for hiding this comment

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

would probably make sense to make the logits_tensor optional here, so you can support PLD with the same code. I assume it's just a matter of not passing it to the ExternalDraftTokensConfig

Copy link
Author

Choose a reason for hiding this comment

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

makes sense, will do this in a new PR, merging these changes into another fork (Nate is working on some rust changes).

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