Repository navigation
Improve ObjectStore ergonomics for end-of-run persistence - #9905
Conversation
|
Read the diff (didn't run the suite). The regression algorithm's own expectation shows how Smaller: |
- 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
66a5aa0 to
e956172
Compare
There was a problem hiding this comment.
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.ndarraywith.tolist(), and pandas objects withto_dict(), or reject them and point tosave_dataframe. - Send .NET objects to the existing Newtonsoft path, so
List,Dictionaryand Lean types serialize the way the C#SaveJson<T>does. - Keep
str()only for the types you list on purpose (Symbol). Anything else raises aTypeErrorthat 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")raisesTypeError: No method matches, because a Pythonstrdoesn't bind toPyObjecthere. Either support it or leave it out of whatsave_jsonclaims to accept.save_dataframeaccepts aSeries, butread_dataframereturns aDataFrame, so a round trip changes the type. Document this or handle it.GetKeyIssueshas 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 useContainsKey, 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.
…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
|
Thanks, each point reproduced with a failing test first, then fixed.
Series round trip. Documented on
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. |
There was a problem hiding this comment.
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 throughtolist(), NaN and infinity becomenull, pandas objects raise and point tosave_dataframe, and any other type raises aTypeErrorthat names it. Each row of my earlier table has a test. SanitizeKeyis documented as lossy, and a test shows the collision. You're right thata b.csvis 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
GetKeyIssuesreasoning 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 compliantfromdumps, not theTypeErroryou 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 becomesnull. 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.
…, 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
|
Thanks, reproduced each case with a failing test first. numpy datetime64 / NaT (blocking). NaN dictionary key. Raises NaN inside .NET objects. Added to the Storage suite: 162 passed. Both regression algorithms pass, and the Python one ran through the Launcher again to completion with no runtime error. |
There was a problem hiding this comment.
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'. KeyToReadchecks_savedKeysfirst, so read-after-write within a run returns the new object. It follows the entry only while the saved object exists, so after aDeletethe original comes back._savedKeysis recorded only after the store accepts the save, so a save rejected by the storage limit doesn't redirect reads.KeyToReadandKeyToSavecheckIsSupportedKey. The issues string is built only for the first-time warning.- Each case has a test:
SavingToAnExistingUnsupportedKeyGoesToTheSanitizedNameWithAWarningEveryTime(one store and a fresh one),DeletingASavedUnsupportedKeyRemovesTheSavedObjectThenTheOriginalandAFailedSaveToAnUnsupportedKeyDoesNotRedirectItsReads.
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:
KeyToReadreads_store.ContainsKey(key)and thenTryRemoves 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.
…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.
|
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.
Storage, Python wrapper, QuantBook, custom live data feed, custom data object store regressions and both |
There was a problem hiding this comment.
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
falsewith a warning. This covers both of the sequences I gave (unsupported, colliding unsupported, first unsupported again; unsupported, the valid sanitized key, unsupported again). ForgetSavesTokeeps_savedKeysaccurate after saves through another key and after deletes.KeyToReadno longer changes the dictionary, so the read-side race I noted is gone.ASaveToTheSanitizedNameFromAnotherKeyEndsTheSilentUpdatesOfAnUnsupportedKeycovers 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
_savedKeysentry, 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,
_savedKeyskeeps 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
SavingToAnExistingUnsupportedKeyGoesToTheSanitizedNameWithAWarningEveryTimeto 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:
TryGetKeyToSavechecks_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.
…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.
|
Thanks. The rule stays, the signal changes: pushed in the latest commit with the tests updated first.
Storage, Python wrapper, QuantBook, custom live data feed, custom data object store regressions and both |
There was a problem hiding this comment.
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", ...)inon_dataor 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. Onmasterthe 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
OnEndOfAlgorithmuntil 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:
- 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. - 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.
- Keep rejecting unsupported keys on save, as
masterdoes, and keep the rest of the PR: reads of existing unsupported keys, the per-issue message,SaveText/ReadTextand 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.
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.
|
Split in two, as of 474b101: this PR now holds only the helper changes ( |
All addressed, review is stale
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:The changes:
PythonObjectStore : ObjectStoreholds the Python-only helpers:SaveJson(path, PyObject)/ReadJson(path)for dicts, lists, tuples and .NET objects withdatetime,Decimal, numpy, NaN and pandas-rejection handling, andSaveDataframe/ReadDataframefor DataFrames and Series as CSV or JSON.AlgorithmPythonWrapper.SetObjectStoreandQuantBook(Python notebooks) build aPythonObjectStore;QCAlgorithm.SetObjectStorekeeps an already wrapped store. C# algorithms keep the plainObjectStore.SaveText/ReadTextaliasSave/Read.LocalObjectStore.ReadByteslists 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
PythonObjectStorehelpers,SaveText/ReadTextand 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/ReadTextas aliases, the missing-key error listing the keys and truncating at ten,QCAlgorithmkeeping a wrapped store,QuantBookbuilding aPythonObjectStorefor Python notebooks.AlgorithmPythonWrapperTests: a Python algorithm gets aPythonObjectStoreandsave_jsonworks throughself.object_store.ObjectStoreHelpersRegressionAlgorithm(C# and Python): text, JSON and DataFrame round trips, the missing-key error.QuantBookTestsandCustomLiveDataFeedTests.Types of changes
Checklist:
bug-<issue#>-<description>orfeature-<issue#>-<description>