Skip to content

Improve ObjectStore ergonomics for end-of-run persistence - #9905

Merged
jhonabreul merged 14 commits into
QuantConnect:masterfrom
jhonabreul:feature-object-store-ergonomics
Nov 4, 2026
Merged

jhonabreul merged 14 commits into
QuantConnect:masterfrom
jhonabreul:feature-object-store-ergonomics

Conversation

@jhonabreul

@jhonabreul jhonabreul commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Description

End-of-run persistence to the ObjectStore is a common pattern, and its most frequent failures surface only in OnEndOfAlgorithm, losing the output of an otherwise green run:

Object of type datetime is not JSON serializable
'ObjectStore' object has no attribute 'save_text' Did you mean: 'save'?
Object with path 'trade_log.csv' was not found in the current project.

The changes:

  • PythonObjectStore : ObjectStore holds the Python-only helpers: SaveJson(path, PyObject) / ReadJson(path) for dicts, lists, tuples and .NET objects with datetime, Decimal, numpy, NaN and pandas-rejection handling, and SaveDataframe / ReadDataframe for DataFrames and Series as CSV or JSON.
  • AlgorithmPythonWrapper.SetObjectStore and QuantBook (Python notebooks) build a PythonObjectStore; QCAlgorithm.SetObjectStore keeps an already wrapped store. C# algorithms keep the plain ObjectStore.
  • SaveText / ReadText alias Save / Read.
  • LocalObjectStore.ReadBytes lists the available keys (up to ten) in the missing-key error, so a typo or a wrong extension is visible in the error itself.

The key rules (sanitizing unsupported keys, the collisions that throw, the warnings) moved to their own PR, #10029.

Related Issue

N/A

Motivation and Context

A completed backtest should not die on its final persistence call over a datetime in a dictionary or a helper that only exists in C#.

Requires Documentation Change

Yes: the Python PythonObjectStore helpers, SaveText / ReadText and the keys listed in the missing-key error.

How Has This Been Tested?

  • LocalObjectStoreTests: one Python test per helper pair, the tolerant types, the rejections with their messages, the DataFrame CSV and JSON round trips and the time index restore.
  • LocalObjectStoreTests: SaveText / ReadText as aliases, the missing-key error listing the keys and truncating at ten, QCAlgorithm keeping a wrapped store, QuantBook building a PythonObjectStore for Python notebooks.
  • AlgorithmPythonWrapperTests: a Python algorithm gets a PythonObjectStore and save_json works through self.object_store.
  • ObjectStoreHelpersRegressionAlgorithm (C# and Python): text, JSON and DataFrame round trips, the missing-key error.
  • Custom data object store regression algorithms, QuantBookTests and CustomLiveDataFeedTests.
  • All of the above in one run: 165 passed, 0 failed.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Refactor (non-breaking change which improves implementation)
  • Performance (non-breaking change which improves performance. Please add associated performance test and results)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Non-functional change (xml comments/documentation/etc)

Checklist:

  • My code follows the code style of this project.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My branch follows the naming convention bug-<issue#>-<description> or feature-<issue#>-<description>

@arhancanli

Copy link
Copy Markdown

Read the diff (didn't run the suite). The regression algorithm's own expectation shows how SanitizeKey behaves: trade_log_ai_hardware_&_cloud.csv becomes trade_log_ai_hardware___cloud.csv, i.e. each unsupported character maps to a single _. That mapping is lossy and many-to-one: a&b.csv, a b.csv and an existing a_b.csv all sanitize to the same key, so two distinct names written in one run silently overwrite each other, the same kind of lost output this PR is meant to prevent. Worth a test that sanitizes two different names to the same key, and either documenting that SanitizeKey is not injective or offering a collision-safe variant (e.g. a short hash suffix when characters were replaced).

Smaller: SaveJson's fallback of str() for other types (Symbol and so on) is read back by ReadJson as a plain string, so a round trip changes the type; the PR text says this for the encoder, but the Python test would be clearer if it asserted what comes back for a Symbol, not just that saving succeeds.

- Key rules (charset/extension) move to static ObjectStore.IsSupportedKey;
  unsupported-key errors now state the rules and point to the new
  ObjectStore.SanitizeKey helper for programmatically-built keys
- Reading a missing key lists the available keys in the error
- New Python-facing helpers: save_json/read_json with a tolerant encoder
  (datetime/date/time, Decimal, numpy scalars, Symbol, non-string dict keys)
  and read_json default for missing keys, save_dataframe (CSV, or JSON for
  .json keys) and save_text as an alias of save
- Unit tests plus ObjectStoreErgonomicsRegressionAlgorithm (C# + Python)
…frame

- ObjectStore.ValidateKey replaces the public SupportedKeyRules and names the
  problem in the key: unsupported characters, extra dots or a bad extension
- The Python ReadJson throws on a missing key like every other read method
- ReadText and ReadDataframe mirror SaveText and SaveDataframe
@jhonabreul
jhonabreul force-pushed the feature-object-store-ergonomics branch from 66a5aa0 to e956172 Compare October 30, 2026 20:22
@jhonabreul jhonabreul added the kronos:review Request review label Oct 30, 2026
@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot added kronos:review:queued review: queued kronos:review:working review: in progress and removed kronos:review Request review kronos:review:queued review: queued labels Oct 30, 2026

@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Requesting changes: one blocking defect in the new Python save_json, plus missing evidence.

CI is green on e956172. I built the branch and called the new Python methods through a local test against LocalObjectStore, then made the same calls on master to compare.

Blocking: save_json(path, obj) reports success but saves the wrong data for common values. On master, a Python save_json(key, x) without a type argument always raises TypeError (no overload matches), so the user knows the save failed. On this branch the call goes to the new PyObject overload. Any value the encoder doesn't recognize is saved as str(value), and the call returns True. What gets saved now:

value passed saved JSON
List[int] [1, 2] (a .NET list) "System.Collections.Generic.List`1[System.Int32]"
Dictionary[str, float] "System.Collections.Generic.Dictionary`2[System.String,System.Double]"
IndicatorDataPoint(...) "2020-01-01T00:00:00 - 5"
{'w': np.array([1.5, 2.5])} {"w": "[1.5 2.5]"}
{'s': pd.Series([1, 2])} {"s": "0 1\n1 2\ndtype: int64"}
{'w': float('nan')} {"w": NaN} (not valid JSON; read_json in Python accepts it, strict JSON readers don't)

A run can now finish green and the user only finds out later, when they read the data back, that the output is gone. Losing that output is what this PR is meant to prevent. Suggested fix:

  • Convert the common types: numpy.ndarray with .tolist(), and pandas objects with to_dict(), or reject them and point to save_dataframe.
  • Send .NET objects to the existing Newtonsoft path, so List, Dictionary and Lean types serialize the way the C# SaveJson<T> does.
  • Keep str() only for the types you list on purpose (Symbol). Anything else raises a TypeError that names the type.
  • Add a Python test for each of the cases above.

An explicit generic call, save_json[List[int]](key, obj), still goes to the generic method and serializes correctly (checked), so existing typed calls are not affected.

Earlier comment not yet addressed: I agree with the open point that SanitizeKey maps many names to one key: a&b.csv, a b.csv and a_b.csv all become a_b.csv, so two names written in the same run silently overwrite each other. At least document this in the XML docs and add a test for it. A variant that can't collide, such as a short hash suffix added when characters were replaced, would be better.

Smaller points (non-blocking):

  • save_json(key, "text") raises TypeError: No method matches, because a Python str doesn't bind to PyObject here. Either support it or leave it out of what save_json claims to accept.
  • save_dataframe accepts a Series, but read_dataframe returns a DataFrame, so a round trip changes the type. Document this or handle it.
  • GetKeyIssues has a single call site. The repository standard is to inline single-use methods.
  • When a read misses, the error lists the keys by calling Keys, which walks the store's entries. That only happens on the error path, so it's fine. Code that probes keys with try/except should use ContainsKey, as the message already says.

Evidence: the unit tests and the regression algorithm cover the expected paths only, which is how the case above slipped through. This changes what Lean's algorithm API does for Python users. Please also run a Python algorithm end to end, the way users run it, that uses save_json/read_json, save_dataframe/read_dataframe, save_text and sanitize_key in on_end_of_algorithm. Include values from the table above, then add the outcome to the description.

Other repositories: the Python stubs are generated, but the documentation needs the new ObjectStore members and the new error messages, as the description says.

Risk label: risk:high. This PR adds members to Lean's user-facing algorithm API (SaveJson/ReadJson for Python objects, SaveDataframe/ReadDataframe, SaveText/ReadText, IsSupportedKey/ValidateKey/SanitizeKey), and changes the messages of existing errors. Additions to the algorithm API are high risk however they're tested.

Comment thread Common/Storage/ObjectStore.cs Resolved
Comment thread Common/Storage/ObjectStore.cs Resolved
@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot added risk:high High risk and removed kronos:review:working review: in progress labels Oct 30, 2026
…SanitizeKey

- The Python helper is an allow-list: numpy arrays via tolist, NaN and
  infinity as null, .NET objects through Newtonsoft like SaveJson<T>,
  pandas rejected pointing to SaveDataframe, other types raise TypeError
- Top-level Python scalars are documented as not accepted
- SanitizeKey is documented as lossy, with a collision test
- ReadDataframe documents the Series round trip
@jhonabreul

Copy link
Copy Markdown
Collaborator Author

Thanks, each point reproduced with a failing test first, then fixed.

save_json fallback (blocking). str() is gone. _normalize is now an allow-list: datetime/date/time as ISO-8601, Decimal and numpy scalars as numbers, numpy arrays via tolist(), NaN and infinity as null, .NET objects (Symbol included) through Newtonsoft so they match the C# SaveJson<T>, non-string keys as strings. pandas objects raise TypeError pointing to save_dataframe; any other type raises TypeError naming it. One Python unit test per row of your table, all red before the change.

SanitizeKey collisions. Documented as lossy in the XML docs, with SanitizeKeyIsLossyAndDistinctNamesCanCollide showing two names mapping to one key and the second save overwriting the first. No hash suffix: it only sanitizes, collision checks stay with the caller. One correction: a b.csv is a valid key (space is allowed) and is returned unchanged.

save_json(key, "text"). The binder rejects a Python primitive for a PyObject parameter when overloads exist, a pythonnet matter. For this PR the docs narrow the claim: dict, list, tuple or .NET object; top-level scalars raise and save_text is the way to store text. A test pins the rejection.

Series round trip. Documented on ReadDataframe: a saved Series comes back as a one-column DataFrame, squeeze() gives the Series.

GetKeyIssues. Kept as a private helper. It is ~40 lines of diagnosis with one three-line caller; inlining it would make ValidateKey the longest method in the class.

Evidence. The Python regression algorithm now includes a .NET list, a numpy array with NaN, a Symbol value and key, and a rejected pandas Series, and ran through the Launcher to completion with no runtime error. Full storage suite: 159 passed.

@jhonabreul jhonabreul added the kronos:review Request review label Oct 30, 2026
@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot added kronos:review:queued review: queued kronos:review:working review: in progress and removed kronos:review Request review kronos:review:queued review: queued labels Oct 30, 2026

@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Requesting changes for one remaining gap in the save_json allow-list. Everything else from my last review is resolved.

CI is green on 2405d2c. I built the branch and ran the storage tests locally: 173 passed. I also ran a throwaway probe test against LocalObjectStore for the edge cases below.

Resolved since my last review:

  • The str() fallback is gone. .NET objects go through Newtonsoft, numpy arrays through tolist(), NaN and infinity become null, pandas objects raise and point to save_dataframe, and any other type raises a TypeError that names it. Each row of my earlier table has a test.
  • SanitizeKey is documented as lossy, and a test shows the collision. You're right that a b.csv is a valid key (space is in the allowed set), so my earlier example was wrong about that one.
  • The top-level scalar rejection, the Series round trip and the GetKeyIssues reasoning are all fine with me.
  • The Python regression algorithm now covers the new cases, and the description reports an end-to-end run with no runtime error.

Blocking: numpy datetime values still save as something other than what was passed, and the call returns True. numpy.datetime64 is a numpy.generic, so .item() / .tolist() turns a nanosecond-precision value into an integer. pandas uses nanosecond precision by default. What my probe saved:

value passed saved JSON
{'t': pd.to_datetime(['2020-01-01']).values} (e.g. history.index.values) {"t": [1577836800000000000]}
{'t': np.datetime64('2020-01-01T00:00:00.000000000')} {"t": 1577836800000000000}
{'t': pd.NaT} {"t": "NaT"}

A Python datetime and a pd.Timestamp both save as ISO-8601, as documented. A numpy datetime64 should do the same, or be rejected, so it doesn't save as an integer. That keeps the docstring's promise that any type that would not round trip is rejected. Suggested fix: convert numpy.datetime64 values and datetime64 arrays to ISO-8601, for example through pandas.Timestamp(v).isoformat(), or with numpy.datetime_as_string for arrays. Save NaT as null, the same as NaN. Either convert numpy.timedelta64 or reject it, the way a Python timedelta is rejected today. Please add a test for each case.

Smaller points (non-blocking):

  • A NaN dictionary key raises ValueError: Out of range float values are not JSON compliant from dumps, not the TypeError you use everywhere else. This is a rare case, so I'm only noting it.
  • A NaN inside a .NET object is written as the string "NaN", which is Newtonsoft's default, while a Python NaN becomes null. That matches the documentation (".NET objects as the C# SaveJson<T> overload writes them"), so it's fine, but it may be worth one line in the user docs.

Other repositories: the Python stubs are generated. The documentation still needs the new ObjectStore members and error messages, as the description says.

Risk label: risk:high, unchanged. This PR adds members to Lean's user-facing algorithm API (SaveJson/ReadJson for Python objects, SaveDataframe/ReadDataframe, SaveText/ReadText, IsSupportedKey/ValidateKey/SanitizeKey) and changes the messages of existing errors. Additions to the algorithm API are high risk however they're tested.

Comment thread Common/Storage/ObjectStore.cs Resolved
@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot removed the kronos:review:working review: in progress label Oct 30, 2026
…, NaT as null

- datetime64 scalars and arrays convert through pandas.Timestamp, so
  nanosecond values no longer save as integers
- timedelta, pd.Timedelta and timedelta64 save as ISO-8601 durations
- pd.NaT and NaT values save as null like NaN
- a NaN dictionary key raises TypeError like the other rejections
@jhonabreul

Copy link
Copy Markdown
Collaborator Author

Thanks, reproduced each case with a failing test first.

numpy datetime64 / NaT (blocking). datetime64 scalars now save as ISO-8601 through pandas.Timestamp(v).isoformat(), nanoseconds kept; datetime64/timedelta64 arrays are converted element-wise instead of through tolist(), so history.index.values saves as ISO strings; pd.NaT and datetime64('NaT') save as null, like NaN. Python timedelta, pd.Timedelta and timedelta64 now save as ISO-8601 durations (P1DT2H0M0S), which pd.Timedelta parses back; timedelta64('NaT') is null. Tests: PythonSaveJsonWritesNumpyDatetimesAsIso8601AndNatAsNull and PythonSaveJsonWritesTimedeltasAsIso8601Durations, one assertion per case in your table plus a datetime64[D] grid and a pd.Timestamp. The Python regression algorithm also stores a DatetimeIndex.values now.

NaN dictionary key. Raises TypeError: save_json() cannot use nan as a dictionary key like the other rejections. Test: PythonSaveJsonRejectsNanDictionaryKeysWithTypeError.

NaN inside .NET objects. Added to the SaveJson docs: Newtonsoft writes it as the "NaN" string.

Storage suite: 162 passed. Both regression algorithms pass, and the Python one ran through the Launcher again to completion with no runtime error.

@jhonabreul jhonabreul added the kronos:review Request review label Oct 30, 2026
@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot added kronos:review:queued review: queued kronos:review:working review: in progress and removed kronos:review Request review kronos:review:queued review: queued labels Oct 30, 2026
@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot added kronos:review:working review: in progress and removed kronos:review:queued review: queued labels Nov 3, 2026

@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Requesting changes. 07bb4f5 fixes the blocker from my last two reviews. But the self-update exemption it keeps on the overwrite warning still lets one save silently replace another key's object, a save that throws on master.

CI is green on 07bb4f5. I built the branch and ran LocalObjectStoreTests and AlgorithmPythonWrapperTests locally: 176 passed. I also ran a throwaway probe test, described below.

Resolved since my last review:

  • Saving to an unsupported key the store already has as given now warns on every save, within a run and on the next run: the object stored under that name is left as is, this save goes to 'model_tar.gz'.
  • KeyToRead checks _savedKeys first, so read-after-write within a run returns the new object. It follows the entry only while the saved object exists, so after a Delete the original comes back.
  • _savedKeys is recorded only after the store accepts the save, so a save rejected by the storage limit doesn't redirect reads.
  • KeyToRead and KeyToSave check IsSupportedKey. The issues string is built only for the first-time warning.
  • Each case has a test: SavingToAnExistingUnsupportedKeyGoesToTheSanitizedNameWithAWarningEveryTime (one store and a fresh one), DeletingASavedUnsupportedKeyRemovesTheSavedObjectThenTheOriginal and AFailedSaveToAnUnsupportedKeyDoesNotRedirectItsReads.

Blocking: once a store has saved an unsupported key, saving that key again replaces whatever is under its sanitized name, with no warning, even when a different key wrote it in between. The overwrite warning is skipped whenever _savedKeys.ContainsKey(path). That entry says this store once saved path. It doesn't say the object under the sanitized name is still the one it saved. My probe, using one ObjectStore over LocalObjectStore:

call warning result
Save("p&q.csv", "A1") first-time warning written to p_q.csv
Save("p:q.csv", "B1") overwrite warning p_q.csv = B1
Save("p&q.csv", "A2") none p_q.csv = A2, so Read("p:q.csv") returns A2
Save("p_q.csv", "C1") none (valid key) p_q.csv = C1
Save("p&q.csv", "A3") none p_q.csv = A3, so Read("p_q.csv") returns A3

The last two rows are the common shape: an algorithm writes p_q.csv and later p&q.csv in the same run, say once per symbol or per day. The valid key's output is lost with no message. That's the outcome the first-time warning says can't happen ("with a warning only when an object already exists under it or under its sanitized name"). On master the p&q.csv save throws.

Suggested fix: exempt a save only while the object under the sanitized name is still the one this store wrote from the same key. For example, on every successful save to a stored key k from a given path, remove the _savedKeys entries whose value is k and whose key isn't path. _savedKeys only holds unsupported keys, so the scan stays small and doesn't grow with valid keys. Please add a test for both sequences above: unsupported, colliding unsupported, first unsupported again; and unsupported, the valid sanitized key, unsupported again. Each one should warn on the last save.

Non-blocking:

  • KeyToRead reads _store.ContainsKey(key) and then TryRemoves the entry. A concurrent save of the same key that lands between the two loses its entry, so reads fall back to the original until the next save. Algorithms rarely save and read the same key from different threads, so this is only a note.

Other repositories: the Python stubs are generated. The rule that types object_store as PythonObjectStore in the stub generator still has to land. The documentation needs the key rewriting rules, including how existing keys with unsupported names behave, and the PythonObjectStore helpers.

Risk label: risk:high, unchanged. This PR adds to Lean's user-facing algorithm API (SaveText/ReadText, and the Python SaveJson/ReadJson/SaveDataframe/ReadDataframe). It also changes what existing ObjectStore calls do with unsupported keys: saves that used to throw now go to a rewritten key. A person should make the merge decision.

Comment thread Common/Storage/ObjectStore.cs Resolved
@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot removed the kronos:review:working review: in progress label Nov 3, 2026
…m another key

A save of an unsupported key goes to the sanitized key only when nothing is stored under it, or when this store wrote it from the same key. Saves through a different key and deletes forget that pairing, so later saves through the unsupported key are rejected with a warning until the sanitized key is free or written directly.
@jhonabreul

Copy link
Copy Markdown
Collaborator Author

Confirmed with your two sequences as a failing test first, and fixed in the latest commit with a stricter rule than the one you suggested.

  • Collisions are rejected. A save to an unsupported key whose sanitized name already holds an object this store did not last write from that key returns false and warns every time: ObjectStore: key 'p&q.csv' is not supported and its sanitized name 'p_q.csv' already holds another object: the save is rejected. Delete 'p_q.csv' or save to it directly to replace it. No object is replaced through a different key any more, which is the outcome master guarantees by throwing.
  • Self-updates stay silent while the object under the sanitized name is this store's last write from that key. _savedKeys now means exactly that: a successful save to a key drops the entries of other keys pointing at it (ForgetSavesTo), and so does a delete. Reads follow an entry only while its object exists, without mutating, which also removes the read-side race you noted.
  • Across runs nothing ties p_q.csv to p&q.csv, so the next run's save is rejected until the user deletes the object or saves to the sanitized name, as the first-time warning tells them. The existing-unsupported-name case keeps its warning once the sanitized name is free.
  • Tests: ASaveToTheSanitizedNameFromAnotherKeyEndsTheSilentUpdatesOfAnUnsupportedKey covers both sequences (unsupported, colliding unsupported, first again; unsupported, the valid sanitized key, unsupported again). SavingAnUnsupportedKeyWhoseSanitizedNameHoldsAnotherObjectIsRejectedWithAWarningEveryTime replaces the overwrite test, and SavingToAnExistingUnsupportedKeyGoesToTheSanitizedNameWithAWarningEveryTime now has the fresh store rejected first.

Storage, Python wrapper, QuantBook, custom live data feed, custom data object store regressions and both ObjectStoreErgonomicsRegressionAlgorithm variants: 321 passed, 0 failed.

@jhonabreul jhonabreul added the kronos:review Request review label Nov 3, 2026
@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot added kronos:review:queued review: queued kronos:review:working review: in progress and removed kronos:review Request review kronos:review:queued review: queued labels Nov 3, 2026

@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Requesting changes. 003a855 closes the gap from my last review: within one run, a save can no longer replace another key's object without a message. But the new rule rejects too much. It rejects the same key saved again by the next run, which is the end-of-run pattern this PR exists for.

CI is green on 003a855. I built the branch and ran a throwaway probe test against LocalObjectStore, described below.

Resolved since my last review:

  • A save whose sanitized name holds an object another key wrote in this run now returns false with a warning. This covers both of the sequences I gave (unsupported, colliding unsupported, first unsupported again; unsupported, the valid sanitized key, unsupported again).
  • ForgetSavesTo keeps _savedKeys accurate after saves through another key and after deletes. KeyToRead no longer changes the dictionary, so the read-side race I noted is gone.
  • ASaveToTheSanitizedNameFromAnotherKeyEndsTheSilentUpdatesOfAnUnsupportedKey covers both sequences.

Blocking: once an unsupported key has been saved, every later run's save to that key is rejected, and reads return the first run's data. _savedKeys starts empty in each new store. So in the next backtest, or after a live restart, the sanitized object the previous run wrote counts as "another object". My probe used three stores in a row over the same storage, one per run, each doing Save("trade log: AI & Cloud.csv", run) and then Read of the same key:

run Save Read
1 True run1
2 False run1
3 False run1

Runs 2 and 3 log a rejection through Debug and finish green. Most algorithms don't check the bool that Save returns, so from the second run on, the output is silently lost and anything reading the key gets stale data. A live algorithm that saves its state under such a key stops saving after its first restart, then reloads the old state on every restart after that. On master every one of these saves throws, so the user fixes the key. The PR's description example (trade_log_ai_hardware_&_cloud.csv in OnEndOfAlgorithm) works once per project and then stops. SavingToAnExistingUnsupportedKeyGoesToTheSanitizedNameWithAWarningEveryTime asserts this rejection for a fresh store, so it's in the design. The description's "until the sanitized key is free" puts it on the user, but they only see a debug message.

Suggested fix: reject only what this run caused, and warn on the rest.

  • Reject when the sanitized name holds an object another key wrote in this run, meaning it's the target of another key's _savedKeys entry, or this run ended the pairing through a save to that name. That keeps both sequences from my last review rejected.
  • When the object under the sanitized name already existed before this run, overwrite it and warn, as 07bb4f5 did. After that first successful save, _savedKeys keeps that run's later saves quiet, so it's one warning per key per run.
  • Add a test with two or three stores in a row saving the same unsupported key, each save succeeding and each run reading its own data. Update the fresh-store part of SavingToAnExistingUnsupportedKeyGoesToTheSanitizedNameWithAWarningEveryTime to match.

If you'd rather keep this strict for objects that existed before the run, throw as master does, so the run fails visibly. Don't return false with only a debug message.

Non-blocking:

  • TryGetKeyToSave checks _store.ContainsKey(key) and then saves. A concurrent save of the same sanitized name between the two gets around the check. Algorithms rarely do that, so this is only a note.

Other repositories: the Python stubs are generated. The rule that types object_store as PythonObjectStore in the stub generator still has to land. The documentation needs the key rewriting rules, the rejection cases and how existing keys with unsupported names behave, plus the PythonObjectStore helpers.

Risk label: risk:high, unchanged. This PR adds to Lean's user-facing algorithm API (SaveText/ReadText, and the Python SaveJson/ReadJson/SaveDataframe/ReadDataframe). It also changes what existing ObjectStore calls do with unsupported keys: saves that used to throw are now rewritten or rejected with a false return. A person should make the merge decision.

Comment thread Common/Storage/ObjectStore.cs Resolved
@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot removed the kronos:review:working review: in progress label Nov 3, 2026
…her key

Nothing records who wrote the object under a sanitized key: a previous run, another algorithm or an upload. A save through the unsupported key throws instead of returning false, so the run fails visibly rather than losing its output to a debug message.
@jhonabreul

Copy link
Copy Markdown
Collaborator Author

Thanks. The rule stays, the signal changes: pushed in the latest commit with the tests updated first.

  • Why no overwrite across runs: once a store has saved p_q.csv from p&q.csv, nothing records that. In the next run the object under p_q.csv may come from the previous run of this algorithm, from another algorithm, from an upload or from the CLI. Overwriting it through a different key would be the silent replacement your earlier reviews rightly blocked within a run, so a later run is not allowed to replace it either. The decision is to never rewrite an object through a key other than the one it was written with in the current run.
  • Throw instead of false: as you suggest for the strict option, a sanitized save whose sanitized key already holds an object this run did not write from that key now throws ArgumentException: ObjectStore: key 'trade log: AI & Cloud.csv' is not supported and its sanitized name 'trade log_ AI _ Cloud.csv' already holds another object. Delete 'trade log_ AI _ Cloud.csv' or save to it directly to replace it. The run fails visibly, on the second run at the latest, where master fails on the first. The same exception covers the in-run collisions from your previous review, one rule for both.
  • Same-key saves within a run stay silent after the first-time warning, and the exact sanitized key is a supported key, so the user replaces the object through it.
  • Tests: ANewRunThrowsOnSavingAnUnsupportedKeyAPreviousRunSanitized runs your probe with two stores in a row, the second throwing, reading the first run's object, saving through the sanitized key and throwing again on the unsupported one. SavingAnUnsupportedKeyWhoseSanitizedNameHoldsAnotherObjectThrowsEveryTime and ASaveToTheSanitizedNameFromAnotherKeyEndsTheSilentUpdatesOfAnUnsupportedKey assert the exception for the in-run cases, and the fresh-store part of SavingToAnExistingUnsupportedKeyGoesToTheSanitizedNameWithAWarningEveryTime throws until the sanitized key is free.
  • Concurrent saves of the same sanitized key: not a supported pattern, left as is.

Storage, Python wrapper, QuantBook, custom live data feed, custom data object store regressions and both ObjectStoreErgonomicsRegressionAlgorithm variants: 322 passed, 0 failed.

@jhonabreul jhonabreul added the kronos:review Request review label Nov 3, 2026
@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot added kronos:review:queued review: queued kronos:review:working review: in progress and removed kronos:review Request review kronos:review:queued review: queued labels Nov 3, 2026

@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Requesting changes. 3c973ad swaps the false return for an ArgumentException, which I offered as an option last time, so nothing fails silently any more. Thinking it through, though, the strict rule now creates a failure master doesn't have: a live algorithm that ran fine can stop on its next restart. And the PR's motivating case works only on a project's first run.

CI on 3c973ad was still running when I posted this: one build check passed, one was skipped and the rest were pending. I built the branch and ran LocalObjectStoreTests and AlgorithmPythonWrapperTests locally: 178 passed, 0 failed.

Resolved since my last review: a save whose sanitized name holds an object this run didn't write from the same key now throws every time, both within a run and across runs. So it can't be lost quietly behind a false that nobody checks. ANewRunThrowsOnSavingAnUnsupportedKeyAPreviousRunSanitized covers my two-run probe, and the in-run collision tests now assert the exception. The class doc, the first-time warning and the description all match the new behavior.

Blocking: an unsupported key with a fixed name now fails on the second run instead of the first, and for a live algorithm that means at a restart nobody is watching. ANewRunThrowsOnSavingAnUnsupportedKeyAPreviousRunSanitized pins the sequence: run 1 Save("trade log: AI & Cloud.csv") returns True, and run 2's save of the same key throws. What that means for users:

  • Live state saves. A common pattern is if self.live_mode: self.object_store.save("state: SPY.json", ...) in on_data or on a schedule. The first deployment works and the algorithm runs for days or weeks. After its first restart, whether a redeploy, a crash recovery or a server move, the first save throws and the algorithm stops. On master the same save throws within minutes of the first deployment, while the user is still watching. The key never changes, so moving the failure from deploy time to an unattended restart is a new way for a live algorithm to stop.
  • End-of-run persistence, the PR's motivating case. The description says "A completed backtest should not die on its final persistence call over a key spelling". With a fixed key, that now holds only for the project's first backtest. Every backtest after that dies in OnEndOfAlgorithm until the user deletes the object or renames the key. The regression algorithms only pass on repeated runs because they delete their keys at the end, which user algorithms don't do.

I agree with your point that an object written before this run can't be traced to the key that wrote it. But the choice now is between "succeeds once, then always throws" and "always throws", and master's version fails earlier and more predictably. Options, in the order I'd suggest:

  1. Make the sanitized name traceable to its key, so that an object under it can be assumed to come from the same key, and overwrite it then with a warning. For example, use an escape that valid keys won't produce by accident (an unsupported character becomes something like _x26_, not a bare _) and overwrite only when the stored name is that encoding of this key. Collisions with valid keys become practically impossible, and repeated runs keep working.
  2. Overwrite an object that already existed before the run, with a warning on each run (what I suggested last time). It's simple, but it carries the provenance risk you described.
  3. Keep rejecting unsupported keys on save, as master does, and keep the rest of the PR: reads of existing unsupported keys, the per-issue message, SaveText/ReadText and the Python helpers. That way the first run fails the same way every later run would.

If you'd rather keep the current rule, that's a product decision, and a person should make it with the live-restart case in mind. Either way, please add a test for the option you choose that runs the same fixed unsupported key through two or three stores in a row, so the cross-run behavior is pinned whichever way it goes.

Non-blocking: KeyToSave calls WarnOnce before it decides to throw. So on a store's first unsupported key, the log says Using '<sanitized>' instead and then the save throws. The two messages contradict each other. Checking the collision before warning, or not counting a rejected save as the store's one warning, would avoid that.

Other repositories: the Python stubs are generated, but the stub generator's rule that types object_store as PythonObjectStore still has to land. The documentation needs the key rewriting rules, the cases that throw (including across runs), how existing keys with unsupported names behave, and the PythonObjectStore helpers.

Risk label: risk:high, unchanged. This PR adds to Lean's user-facing algorithm API (SaveText/ReadText, and the Python SaveJson/ReadJson/SaveDataframe/ReadDataframe). It also changes what existing ObjectStore saves do with unsupported keys: they're now rewritten, and they throw depending on what earlier runs stored. A person should make the merge decision.

Comment thread Common/Storage/ObjectStore.cs Resolved
Comment thread Common/Storage/ObjectStore.cs Resolved
@quantconnect-kronos-reviewer quantconnect-kronos-reviewer Bot removed the kronos:review:working review: in progress label Nov 3, 2026
The ObjectStore keeps the Python helpers, the SaveText/ReadText aliases and
the keys listed in the missing-key error. Sanitizing unsupported keys, the
collisions that throw, the warnings and the LocalObjectStore backstop move
to a separate branch off master.
@jhonabreul

Copy link
Copy Markdown
Collaborator Author

Split in two, as of 474b101: this PR now holds only the helper changes (PythonObjectStore with the Python helpers, SaveText / ReadText, the keys listed in the missing-key error). The key rules (sanitizing unsupported keys, the collisions that throw, the warnings and the LocalObjectStore backstop) moved to #10029, branched off master with their tests and a regression algorithm of their own. The description is updated; the review threads about the key rules continue there.

@jhonabreul
jhonabreul merged commit 49b4aa7 into QuantConnect:master Nov 4, 2026
8 checks passed
@jhonabreul
jhonabreul deleted the feature-object-store-ergonomics branch November 4, 2026 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:high High risk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants