fix: removed the file-based roundtrip in LocalREPL.add_context() #53
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.
While looking into LocalREPL.add_context() during the investigation of how rlm works, I noticed it does an extra temp-file roundtrip for every context payload. It looks like it might have been an attempt at “lazy loading”, but in practice it only adds overhead (the data is still fully read back into memory).
The Problem
The previous implementation wrote context to a temp file and then immediately read it back into memory:
This approach:
The Fix
Store context directly in memory, using deep copy for isolation (as expected by the SupportsPersistence protocol):
Performance Impact
Design Note
I noticed the previous temp-file roundtrip looks like it was aiming at “lazy loading”, but in practice it wasn’t.
If we ever want real lazy / file-backed contexts, that’s a bigger follow-up: today the prompt/examples (and tests) assume the context is an in-memory object you can len(), slice, iterate, call string methods on, etc. To make lazy loading work properly we’d need either a file-backed proxy that preserves those semantics, or a separate API like add_context_from_file(...) + updated prompt/examples/tests that teach the model to read/stream from a path.
This PR just removes the misleading roundtrip that didn’t actually provide lazy loading benefits.