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

Implement non-blocking CLI history auto saver #1156

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

teocns
Copy link

@teocns teocns commented Mar 31, 2024

Describe the changes you have made:

Implement history_autosaver utility function to start a background thread for autosaving the CLI history using readline. If readline is unavailable, it returns a no-op class object instead. This ensures the application remains functional, with or without readline.

  • autosaver is constructed before the terminal_interface loop starts, and history messages are queued via the handle's .add() method.

  • terminal_interface.py is now decoupled from readline

  • pyreadline might be a readline alternative for other OS-es

Extra

  • cli_input() handles both single and multi-line inputs by implementing a new argument multi_line, false by default:
def cli_input(prompt: str = "", multi_line=False) -> str

Reference any relevant issues (e.g. "Fixes #000"):

N/A

Pre-Submission Checklist (optional but appreciated):

  • I have included relevant documentation updates (stored in /docs)
  • I have read docs/CONTRIBUTING.md
  • I have read docs/ROADMAP.md

OS Tests (optional but appreciated):

  • Tested on Windows
  • Tested on MacOS
  • Tested on Linux

@teocns
Copy link
Author

teocns commented Mar 31, 2024

Ideally this shouldn't be involving the complexity added by another thread, but I am yet to completely figure out the TUI loop/routine lifecycle;

In theory, file writing is primarily an I/O bound operation, hence co-routines can be leveraged to avoid blocking the main thread. However, the terminal interface is not implemented with the asyncio.

On the other hand, these same file writes occurring within the same thread may also go unnoticed for the following:

  • writes occur right after users send message (prompt);
  • serial chat model system: can't be spammed with a send & recv message flow
  • history size is (or can be trimmed to be) relatively small (a few Kbs)

What are others' thoughts on this?

@KillianLucas
Copy link
Collaborator

KillianLucas commented Apr 7, 2024

Good lord I'm excited for this. Great work @teocns. Always wanted arrow up, especially for debugging, I hate typing the same message over and over again.

I think it would be fine to not be a thread, and just take a tiny but of time to add the user's message. That should take barely any time.

Another thought: we do already save /conversations, so perhaps this should instead just access the last 10 user messages or something? Once at the start? Then just add to that list during the session?

Let me know your thoughts, will just merge if you think those^ changes are worse / only marginally better. I think it would work well either way.

@teocns
Copy link
Author

teocns commented Apr 7, 2024

I think it would be fine to not be a thread, and just take a tiny but of time to add the user's message. That should take barely any time.

Absolutely - an optimistic average byte size per prompt floats around 64KB for a 128K tokens context window (UTF-8 chars range between 1-4 bytes).

Another thought: we do already save /conversations

Good catch! The conversations historical data is buffered in memory when launching OI with --conversations param. I was hoping we could leverage at least the data access implementation from "conversations", but with a brief look at the implementation it looks too monolithic / use-case oriented... 😅

My two cents: being an I/O-bound-first appllication, and considered the open source scale of the project, I would prioritize OI's refactoring to implement an asynchronous presentation layer (just like the FastAPI server) - to begin with. That'd solve the core issue now and will prevent many more in the future

@teocns
Copy link
Author

teocns commented Apr 7, 2024

Let me know your thoughts, will just merge if you think those^ changes are worse / only marginally better. I think it would work well either way.

Whatever aligns more with the vision and roadmap 👍 I'd buy into this implementation for being efficient, although it comes with the cost of having a "thread" which sounds heavy. Threads can indeed behave funny especially within poorly orchestrated multi-threaded ecosystems, or be draining resources when iteratively spawned at high rates... which doesn't seem to be our case

@fire17
Copy link

fire17 commented May 8, 2024

Any ideas on when this will be merged?
Much awaiting readline history :)

Regarding threading:
Since it's very light weight operation, I'd open 1 thread to handle saving readline history. From the main thread push to deque, and pop left in the save-readline thread. This way it's non blocking but also doesn't spawn more than once.
At least those are my thoughts & recommendation.

All the best!

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.

3 participants