-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: auto-detect Console width #3327
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: Doug Edgar <[email protected]>
Hi @rhdedgar! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
I believe we might be getting a line break in the server logs now so this check https://github.com/llamastack/llama-stack/blob/main/.github/workflows/integration-auth-tests.yml#L95 fails :/ |
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.
Until this is resolved #3327 (comment)
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.
+1 once the test is fixed
@leseb tests are passing |
cat $run_dir/run.yaml | ||
# avoid line breaks in the server log, especially because we grep it below. | ||
export COLUMNS=1984 |
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.
thanks, that does fix the current test, but I wonder whether it wouldn't make more sense to make the test assertions more robust to don't make exact string matches but match only on the parts that are important, to avoid breaking on formatting or rewording changes ? Messages aren't stable APIs.
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.
While I agree with the approach in general, I'm not sure how we could relax that grep
line to do a contains
versus a strict match:
grep -q "Enabling authentication with provider: ${{ matrix.auth-provider }}" server.log
What does this PR do?
Addresses Issue #3271 - "Starting LLS server locally on a terminal with 120 chars width results in an output with empty lines".
This removes the specific 150-character width limit specified for the Console, and will now auto-detect the terminal width instead. Now the formatting of Console output is consistent across different sizes of terminal windows.
Closes #3271
Test Plan
Launching the server with several different sizes of terminal windows results in Console output without unexpected spacing. e.g.
python -m llama_stack.core.server.server /tmp/run.yaml --port 8321