Skip to content

Issue #315: Display full completion in CLI #1068

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.

Sign up for GitHub

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

Jump to bottom
Closed
bnkc wants to merge 1 commit into from
Closed

Issue #315: Display full completion in CLI #1068

bnkc wants to merge 1 commit into from

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

rattrayalex commented Jan 13, 2024

@backus is this what you were looking for?

@bnkc
Copy link
Author

bnkc commented Feb 8, 2024

Any news on this one?

@rattrayalex rattrayalex mentioned this pull request Feb 8, 2024
Open
@admsev
Copy link

admsev commented May 26, 2024

@gooroodev please review

@gooroodev
Copy link

gooroodev commented May 26, 2024

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!

rattrayalex
rattrayalex reviewed May 26, 2024
View reviewed changes
src/openai/cli/_api/chat/completions.py

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

@rattrayalex rattrayalex May 26, 2024

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())

rattrayalex
rattrayalex requested changes May 26, 2024
View reviewed changes
Copy link
Collaborator

@rattrayalex rattrayalex left a comment •
edited
Loading

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.

stainless-app bot pushed a commit that referenced this pull request Mar 27, 2025
@RobertCraigie @jacobzim-stl @stainless-app
fix(examples): handle usage chunk in tool call streaming (#1068)
90c2da5 
---------

Co-authored-by: Jacob Zimmerman <[email protected]>
@stainless-app stainless-app bot mentioned this pull request Mar 27, 2025
Merged
@bnkc bnkc closed this by deleting the head repository Jul 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rattrayalex rattrayalex rattrayalex requested changes

@RobertCraigie RobertCraigie Awaiting requested review from RobertCraigie

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bnkc @rattrayalex @admsev @gooroodev