Skip to content
This repository was archived by the owner on Apr 23, 2026. It is now read-only.

add -nd option for chat - #2780

Merged
mergify[bot] merged 2 commits into
instructlab:mainfrom
reidliu41:no-decoration-chat
Jan 29, 2025
Merged

add -nd option for chat#2780
mergify[bot] merged 2 commits into
instructlab:mainfrom
reidliu41:no-decoration-chat

Conversation

@reidliu41

@reidliu41 reidliu41 commented Dec 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • add a option to remove the decoration for the chat

Baed on 2712, add the no decoration mode

$ ilab model chat --help
  -nd, --no-decoration      Disable decorations for chat responses.

$ ilab model chat -nd
Welcome to InstructLab Chat w/ **GRANITE-7B-LAB-Q4_K_M.GGUF** (type `/h` for help)
>>> /h
Help / TL;DR

 • /q: quit
 • /h: show help
 • /a assistant: amend assistant (i.e., model)
 • /c context: change context (available contexts: default, cli_helper)
 • /lc: list contexts
 • /m: toggle multiline (for the next session only)
 • /M: toggle multiline
 • /n: new session
 • /N: new session (ignoring loaded)
 • /d <int>: display previous response based on input, if passed 1 then previous, if 2 then second last
   response and so on.
 • /p <int>: previous response in plain text based on input, if passed 1 then previous, if 2 then second last
   response and so on.
 • /md <int>: previous response in Markdown based on input, if passed 1 then previous, if 2 then second last
   response and so on.
 • /s filepath: save current session to filepath
 • /l filepath: load filepath and start a new session
 • /L filepath: load filepath (permanently) and start a new session

Press Alt (or Meta) and Enter or Esc Enter to end multiline input.

>>> hi
Hello! I'm glad you reached out. I'd be happy to help answer any questions you have.

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the
    conventional commits.
  • Changelog updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Functional tests have been added, if necessary.
  • E2E Workflow tests have been added, if necessary.

@mergify mergify Bot added ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Dec 12, 2024
@reidliu41
reidliu41 force-pushed the no-decoration-chat branch 2 times, most recently from c56c51e to 16b54f4 Compare December 12, 2024 10:51
@mergify mergify Bot added the ci-failure PR has at least one CI failure label Dec 12, 2024
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Dec 12, 2024
@reidliu41 reidliu41 mentioned this pull request Dec 13, 2024
Closed
cdoern
cdoern reviewed Dec 17, 2024
View reviewed changes

@cdoern cdoern left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice, thanks. Ideally we should add some tests for this to ensure both outputs are still ok.

@mergify mergify Bot added the testing Relates to testing label Dec 17, 2024
@reidliu41
reidliu41 requested a review from cdoern December 17, 2024 12:41
@reidliu41

reidliu41 commented Dec 17, 2024

Copy link
Copy Markdown
Contributor Author

@cdoern updated

jaideepr97
jaideepr97 approved these changes Dec 18, 2024
View reviewed changes

@jaideepr97 jaideepr97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks @reidliu41

@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Dec 18, 2024
@ktam3 ktam3 linked an issue Jan 8, 2025 that may be closed by this pull request
ilab model chat "boxes" #2712
Closed
@mergify

mergify Bot commented Jan 24, 2025

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. @reidliu41 please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added needs-rebase This Pull Request needs to be rebased and removed one-approval PR has one approval from a maintainer labels Jan 24, 2025
@reidliu41 reidliu41 added the hold In-progress PR. Tag should be removed before merge. label Jan 24, 2025
@mergify mergify Bot added ci-failure PR has at least one CI failure and removed needs-rebase This Pull Request needs to be rebased ci-failure PR has at least one CI failure labels Jan 24, 2025
@reidliu41
reidliu41 force-pushed the no-decoration-chat branch 2 times, most recently from fc4a806 to fea2e91 Compare January 24, 2025 13:16
@reidliu41

reidliu41 commented Jan 24, 2025

Copy link
Copy Markdown
Contributor Author

Hi, @dmartinol @anastasds, if you have time, could you please help to make a simple test for this change with rag function? No changes with rag, just want to make sure the everything is fine after the change. thanks a lot.
e.g.

ilab model chat --rag

ilab model chat --rag -nd

@dmartinol

dmartinol commented Jan 24, 2025

Copy link
Copy Markdown
Contributor

Hi, @dmartinol @anastasds, if you have time, could you please help to make a simple test for this change with rag function? No changes with rag, just want to make sure the everything is fine after the change. thanks a lot. e.g.

Hey @reidliu41 I manually verified it works without decorations: if you want an automated test we also need to mock the DocumentRetriever and, AFAIK there is no other test mocking it.
I think the changes you did to test_model_chat already seem to cover the no-decoration case pretty well.

@reidliu41

reidliu41 commented Jan 25, 2025

Copy link
Copy Markdown
Contributor Author

Hi, @dmartinol @anastasds, if you have time, could you please help to make a simple test for this change with rag function? No changes with rag, just want to make sure the everything is fine after the change. thanks a lot. e.g.

Hey @reidliu41 I manually verified it works without decorations: if you want an automated test we also need to mock the DocumentRetriever and, AFAIK there is no other test mocking it. I think the changes you did to test_model_chat already seem to cover the no-decoration case pretty well.

Thank you so much for you help and feedback. Yes, it cover the test for no-decoration.

@reidliu41
reidliu41 force-pushed the no-decoration-chat branch 2 times, most recently from 022f26f to ace6f75 Compare January 29, 2025 02:37
@mergify mergify Bot added documentation Improvements or additions to documentation ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jan 29, 2025
reid_liu
add --no-decoration option for chat
68976e4 
- add a new flag to chat without decoration

Signed-off-by: reid_liu <guliu@redhat.com>
reid_liu
update in CHANGELOG.md
c1f4d7a 
Signed-off-by: reid_liu <guliu@redhat.com>
@reidliu41
reidliu41 removed the request for review from cdoern January 29, 2025 11:49
@reidliu41 reidliu41 removed the hold In-progress PR. Tag should be removed before merge. label Jan 29, 2025
@mergify
mergify Bot merged commit 2bf5e50 into instructlab:main Jan 29, 2025
@booxter booxter mentioned this pull request Feb 24, 2025
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers
@cdoern cdoern cdoern left review comments
@jaideepr97 jaideepr97 jaideepr97 approved these changes
@nathan-weinberg nathan-weinberg nathan-weinberg approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

documentation Improvements or additions to documentation testing Relates to testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ilab model chat "boxes"

5 participants

@reidliu41 @dmartinol @jaideepr97 @cdoern @nathan-weinberg