Call the python reset from PythonIndicator.Reset - #9698
Conversation
There was a problem hiding this comment.
Hey @mkzung! Thanks, leaving a few comments
Fix works for the plain non-inheriting case, but two confirmed regressions: segfault via super().reset() in inheriting classes, and uncaught AttributeError from the HasAttr/GetPythonMethod name asymmetry. Resolving the reset method once in SetIndicator + a reentrancy guard would cover most findings — details inline.
| { | ||
| using (Py.GIL()) | ||
| { | ||
| _indicatorWrapper.GetMethod(nameof(Reset), pythonOnly: true)?.Invoke().Dispose(); |
There was a problem hiding this comment.
For inheriting classes _indicatorWrapper wraps the instance itself, so a subclass reset() calling super().reset() (the pre-PR correct pattern) recurses: C# Reset() → python reset() → CLR binding → C# Reset()... Reproduced on this branch: ~995 frames, fatal 0xC0000005. A reentrancy guard is needed here.
There was a problem hiding this comment.
Confirmed, and it is on the normal path. WrapPythonIndicator calls TryConvert, which for an inheriting class hands back that object's own C# part, then points SetIndicator at the same object.
I counted the depth in python rather than reading the crash. The test increments a counter on entry to reset() and stops calling super() at 200, so re-entry arrives as a number instead of taking the host down. On the previous commit it reaches the cap. It is 1 now, and zero would mean the python reset was never reached, so one assertion covers both directions.
The ~995 frames you saw are that same loop without the cap.
Reset invokes the python side once and lets the re-entrant call fall through to the base. The flag is saved and restored rather than cleared, so a reset() calling super() twice cannot re-arm it.
| public override void Reset() | ||
| { | ||
| // GetMethod throws when the attribute is absent, and returns null when it is CSharp | ||
| if (_indicatorWrapper != null && _indicatorWrapper.HasAttr(nameof(Reset))) |
There was a problem hiding this comment.
HasAttr is snake-case tolerant but GetPythonMethod falls back to PascalCase-only GetAttr("Reset") — so self.reset = False (non-method, no Reset) passes the guard then throws an uncaught AttributeError; worked before this PR. Also, a non-bound-method callable (self.Reset = lambda: ...) is silently skipped. Suggest resolving the reset method once in SetIndicator, like _pythonIsReadyProperty — also removes the per-call HasAttr GIL round-trip and avoids caching null under "Reset" in _pythonMethods (keyed by name only, ignores pythonOnly).
There was a problem hiding this comment.
Fixed the way you suggested, and the shape is not new here: AlgorithmPythonWrapper resolves OnData and OnMarginCall into fields at construction.
Resolution happens once in SetIndicator through GetPythonMethodWithChecks, snake-case first and PascalCase second, both behind HasAttr. So self.reset = False now resolves to null rather than reaching GetAttr("Reset"), and null no longer lands in _pythonMethods under a key that ignores pythonOnly. The per-call round trip goes with it.
The non-bound callable I have left alone, deliberately.
That <class 'method'> test inside GetPythonMethod is doing real work: it is what separates a python override from the inherited C# binding. Accept any callable and every inheriting class starts looking like it overrides reset. Two wrappers call the helper directly and BasePythonWrapper routes every InvokeMethod through it, so I would rather widen it on its own.
One consequence of resolving once, since you were the one who flagged the caching: repeated SetIndicator now has something to release. GetIndicatorAsManagedObject calls it from twenty sites in IndicatorExtensions with no caching, unlike WrapPythonIndicator which keys off the handle, so the field is disposed before it is replaced. I stopped there. AlgorithmPythonWrapper releases its equivalents in Dispose, but PythonIndicator has no Dispose and does not release _instance or _indicatorWrapper either, and adding one is a lifetime contract rather than a fix.
| _indicatorWrapper.GetMethod(nameof(Reset), pythonOnly: true)?.Invoke().Dispose(); | ||
| } | ||
| } | ||
| _isReady = false; |
There was a problem hiding this comment.
If the python reset() raises, _isReady = false / base.Reset() are skipped → half-reset indicator. Run them before the invoke or in a finally.
There was a problem hiding this comment.
Confirmed. Both are in a finally now.
| indicator.Reset(); | ||
| indicator.Update(new IndicatorDataPoint(reference, 100m)); | ||
|
|
||
| Assert.AreEqual(100m, indicator.Current.Value); |
There was a problem hiding this comment.
Also assert IsReady is false (or Samples == 0) after Reset() — dropping _isReady = false wouldn't fail this test.
There was a problem hiding this comment.
Sharper than it first looked, because the assertion would have been vacuous as well as missing. The indicator has period 14 and the test fed it three points, so it was never ready, and asserting IsReady is false after a reset would have passed against any implementation at all.
It feeds 20 now and asserts ready first.
Dropping _isReady = false turns it red in all six fixtures that inherit PythonIndicatorTests.
A subclass reset() calling super().reset() came back through the CLR binding and
recursed. Resolving the method in SetIndicator also stops a non-method reset
attribute reaching GetAttr("Reset"), and the base reset now runs even when the
python side raises.
|
All four reproduced and fixed. Second commit rather than an amend, so these threads stay attached. The three new cases build their own python classes and never touch Five runs fail on the previous commit, none on this one. The new file adds no analyzer warning beyond the two CA1515 that every public fixture here already emits. A caveat instead of a clean number for the wider suite. |
2 participants
Description
PythonIndicator never overrode Reset(), so a reset defined in python was never called and _isReady stayed set.
Related Issue
#9697
Motivation and Context
indicator_history resets before it replays, so it returned values mixed with whatever the indicator had already seen. Same class as #9686, #9687, #9688 and #9694, one level up.
Requires Documentation Change
No.
How Has This Been Tested?
Added ResetClearsTheStateHeldInPython to PythonIndicatorTests. It runs in all six python fixtures, both wrapping paths and both naming conventions, and fails on master at 100.75 against 100. The two duck-typed fixtures gained the reset they were missing.
Full suite,
--filter "TestCategory!=TravisExclude&TestCategory!=ResearchRegressionTests", run on pristine master and on this branch:Same three, and they fail on master as well: CommanCallback(Python), ThreadSafety, ZipBytesReturnsByteArrayWithCorrectLength. The extra six are the new test in its six fixtures.
One note on the guard: GetMethod throws when the attribute is missing altogether and returns null only when it resolves to C#, so an unguarded call breaks every python indicator that has no reset. The first version of this did exactly that, and only the full run caught it.
Types of changes
Checklist:
bug-<issue#>-<description>