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

[doc] custom output formatter schema #2018

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

sindhuvahinis
Copy link
Contributor

Description

Brief description of what this PR is about

  • If this change is a backward incompatible change, why must this change be made?
  • Interesting edge cases to note here

@sindhuvahinis sindhuvahinis requested review from zachgk, frankfliu and a team as code owners June 4, 2024 18:45
@sindhuvahinis sindhuvahinis merged commit b8c2fec into deepjavalibrary:master Jun 4, 2024
2 checks passed
+RequestInput input
}
class TextGenerationOutput{
+map sequences
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the mapping?


class RequestInput{
+int request_id
+dict parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

we used map previously, and dict here. Let's keep it consistent (or maybe i'm not understanding the difference between the two).

If they are the same, should we say what the mapping is? (from what to what)

from djl_python.output_formatter import TextGenerationOutput
import json

def custom_output_formatter(request_output: TextGenerationOutput):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: let's add the return type here -> str to keep it consistent with the method signature we explain at the top


"""
best_sequence = request_output.sequences[request_output.best_sequence_index]
next_token, first_token, last_token = best_sequence.get_next_token()
Copy link
Contributor

Choose a reason for hiding this comment

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

out of scope here, but it seems confusing that get_next_token returns 3 token values? i'll connect with you later to understand this more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it might be more clear if its next_token, is_first_token, is_last_token

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.

3 participants