Enable readline for the chat prompt and persist history - #1654
Open
Vlor999 wants to merge 1 commit into
Open
Conversation
corridor_input() wraps its ANSI escapes in \x01/\x02, the readline markers for non-printing characters, but nothing imported readline. Without it those two bytes were written to the terminal verbatim, and the prompt had no history or line editing. Import readline (guarded, it is absent on some platforms) and load/save ~/.mlx_lm_chat_history around the chat session, on rank 0 only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Fixes #1653.
corridor_input()wraps its ANSI escapes in\x01/\x02— readline'smarkers for non-printing characters — but nothing imported
readline, so thosetwo bytes were written to the terminal verbatim and the prompt had no history or
line editing.
This imports
readline(guarded, it is absent on some platforms) andloads/saves
~/.mlx_lm_chat_historyaround the chat session, on rank 0 only.Why
Under a real PTY (reproducer in #1653):
0x01emitted0x02emittedThe markers already in
corridor_input()are evidence this was the intent — theyare inert without
readline.How
import readlinein atry/except ImportError, falling back toNone. Everyhistory helper no-ops when it is
None, so platforms without it keep workingexactly as they do today.
load_chat_history()inChatUI.__enter__,save_chat_history()in__exit__, both behind the existingrank == 0gate so distributed workersnever touch the file.
than raising, so a bad
$HOMEcannot break the chat.Scope — deliberately smaller than #1382
This replaces #1382, which bundled four things and stalled. Rebuilt on current
mainagainst the newerChatUI, keeping only what stands up:broadcast_stringChatUI.prompt()on currentmainis already rank-aware.sample_utilstop_k messagemain354 lines became 96.
Validation
Five tests added: history round-trips through a file, a missing file and an
unwritable path are both non-fatal, everything no-ops when
readlineisNone,and rank 1 never touches the history while rank 0 does.
I checked they are not vacuous by removing the
save_chat_history()call from__exit__: the suite fails. With the change in place, all 44 pass.Note
Ctrl+D still raises
EOFErrorout of the chat loop rather than exitingcleanly. That is a separate small fix and I left it out to keep this focused —
happy to send it after this lands.
Supersedes #1382. Thanks @nastya236 for the review there — the questions about
the distributed path are what led to dropping it. The demo videos from that
thread still show the history/line-editing behaviour this PR keeps.