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

Issue #315: Display full completion in CLI #1068

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

bnkc
Copy link

@bnkc bnkc commented Jan 12, 2024

  • I understand that this repository is auto-generated and my pull request may not be merged

Description

This PR addresses issue #315. The idea is introducing a new --verbose flag to the CLI to display the entire completion response.

Implementation

  • Added argument --verbose to CLI
  • Added verbose arg to CLIChatCompletionCreateArgs
  • Passed verbose_output flag to static method _create
  • If True, dump completion, write to console and return
 if verbose_output:
            sys.stdout.write(completion.model_dump_json(indent=2))
            return

Caveats

Since verbose is not a chat.completions.create argument, we can't pass it to CompletionCreateParams. In my opinion it seems ok to have a flag for now, but I'm sure there's a cleaner way of doing it in the future if new args are introduced that aren't parameters for completions endpoint.

Notes

Do we want this for _stream_create?

Thanks in advance 😀 @RobertCraigie

@bnkc bnkc requested a review from a team as a code owner January 12, 2024 19:15
@bnkc bnkc changed the title Fix issue #315: Display full completion in CLI Issue #315: Display full completion in CLI Jan 12, 2024
@rattrayalex
Copy link
Collaborator

@backus is this what you were looking for?

@bnkc
Copy link
Author

bnkc commented Feb 8, 2024

Any news on this one?

@admsev
Copy link

admsev commented May 26, 2024

@gooroodev please review

@gooroodev
Copy link

Thank you, @admsev, for involving me!

1. Summary of Changes

The pull request introduces the following changes:

  • Adds a new --verbose (-v) option to the CLI for chat completions.
  • When the --verbose option is used, the entire completion response is displayed instead of just the content of the completion.
  • Updates the CLIChatCompletionCreateArgs class to include the verbose attribute.
  • Modifies the _create method in CLIChatCompletion to handle the verbose output.

2. Issues, Bugs, or Typos

Issue 1: Unnecessary Whitespace

In the help string for the --temperature argument, there is an unnecessary newline.

Current Code:

help="""What sampling temperature to use. Higher values means the model will take more risks. Try 0.9 for more creative applications, and 0 (argmax sampling) for ones with a well-defined answer.

Mutually exclusive with `top_p`.""",

Improved Code:

help="""What sampling temperature to use. Higher values means the model will take more risks. Try 0.9 for more creative applications, and 0 (argmax sampling) for ones with a well-defined answer. Mutually exclusive with `top_p`.""",

Issue 2: Import for sys Module

The code uses sys.stdout.write but does not import the sys module.

Improved Code:

import sys

Add this import statement at the beginning of the file.

3. General Review of Code Quality and Style

Code Quality

  • The code is generally well-structured and follows good practices.
  • The addition of the --verbose option is a useful feature that enhances the CLI's functionality.
  • The use of Pydantic's BaseModel for argument parsing is a good practice for type safety and validation.

Code Style

  • The code adheres to PEP 8 standards.
  • The variable names are descriptive and meaningful.
  • The method names and argument names are clear and self-explanatory.

Suggestions for Improvement

  • Consider adding unit tests to cover the new --verbose functionality to ensure it works as expected.
  • Update the documentation to include information about the new --verbose option.

Overall, the changes are well-implemented and improve the functionality of the CLI tool. With the minor issues addressed, the pull request should be ready for merging.

Yours, Gooroo.dev. React or reply to this review with your feedback!


# If verbose output requested, print the entire completion
if verbose_output:
sys.stdout.write(completion.model_dump_json(indent=2))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sys.stdout.write(completion.model_dump_json(indent=2))
sys.stdout.write(completion.to_json())

Copy link
Collaborator

@rattrayalex rattrayalex left a comment

Choose a reason for hiding this comment

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

Thank you for the time spent on this contribution, and for the patience on review!

I think --json would be a better flag than --verbose for this (I'd expect the latter to display more diagnostic information, while --json should produce only JSON to stdout, so it can be piped to other programs like jq.

Furthermore, this flag should work for all requests, not just chat completions.

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.

4 participants