Skip to content

Call the python reset from PythonIndicator.Reset #9698

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.

Sign up for GitHub

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

Open
mkzung wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Call the python reset from PythonIndicator.Reset #9698

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e209155
Call the python reset from PythonIndicator.Reset
mkzung Aug 13, 2026
d0dbc47
Resolve the python reset once and guard against re-entry
mkzung Aug 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions Indicators/PythonIndicator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,23 @@ public override bool IsReady
/// </summary>
public int WarmUpPeriod { get; protected set; }

/// <summary>
/// Resets this indicator to its initial state
/// </summary>
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)))

@Martin-Molinero Martin-Molinero Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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).

@mkzung mkzung Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

{
using (Py.GIL())
{
_indicatorWrapper.GetMethod(nameof(Reset), pythonOnly: true)?.Invoke().Dispose();

@Martin-Molinero Martin-Molinero Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@mkzung mkzung Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

}
}
_isReady = false;

@Martin-Molinero Martin-Molinero Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the python reset() raises, _isReady = false / base.Reset() are skipped → half-reset indicator. Run them before the invoke or in a finally.

@mkzung mkzung Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed. Both are in a finally now.

base.Reset();
}

/// <summary>
/// Computes the next value of this indicator from the given state
/// </summary>
Expand Down
5 changes: 5 additions & 0 deletions Tests/Indicators/PythonIndicatorNoinheritanceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ def __init__(self, name, period):
self.{(SnakeCase ? "value" : "Value")} = sum(self.queue) / count
self.{(SnakeCase ? "is_ready" : "IsReady")} = count == self.queue.maxlen
return self.{(SnakeCase ? "is_ready" : "IsReady")}

def {(SnakeCase ? "reset" : "Reset")}(self):
self.queue.clear()
self.{(SnakeCase ? "value" : "Value")} = 0
self.{(SnakeCase ? "is_ready" : "IsReady")} = False
"
);
var indicator = module.GetAttr("CustomSimpleMovingAverage")
Expand Down
5 changes: 5 additions & 0 deletions Tests/Indicators/PythonIndicatorNoinheritanceTestsLegacy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ def __init__(self, name, period):
count = len(self.queue)
self.{(SnakeCase ? "value" : "Value")} = sum(self.queue) / count
self.{(SnakeCase ? "is_ready" : "IsReady")} = count == self.queue.maxlen

def {(SnakeCase ? "reset" : "Reset")}(self):
self.queue.clear()
self.{(SnakeCase ? "value" : "Value")} = 0
self.{(SnakeCase ? "is_ready" : "IsReady")} = False
"
);
var indicator = module.GetAttr("CustomSimpleMovingAverage")
Expand Down
21 changes: 21 additions & 0 deletions Tests/Indicators/PythonIndicatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ def __init__(self, name, period):
count = len(self.queue)
self.{(SnakeCase ? "value" : "Value")} = np.sum(self.queue) / count
return count == self.queue.maxlen

def {(SnakeCase ? "reset" : "Reset")}(self):
self.queue.clear()
self.{(SnakeCase ? "value" : "Value")} = 0
"
);
var indicator = module.GetAttr("CustomSimpleMovingAverage")
Expand Down Expand Up @@ -165,6 +169,23 @@ public void IsReadyAfterPeriodUpdates()
Assert.IsTrue(sma.IsReady);
}

[Test]
public void ResetClearsTheStateHeldInPython()
{
var indicator = CreateIndicator();
var reference = new DateTime(2024, 1, 1);

for (var i = 0; i < 3; i++)
{
indicator.Update(new IndicatorDataPoint(reference.AddDays(i), 100m + i));
}

indicator.Reset();
indicator.Update(new IndicatorDataPoint(reference, 100m));

Assert.AreEqual(100m, indicator.Current.Value);

@Martin-Molinero Martin-Molinero Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also assert IsReady is false (or Samples == 0) after Reset() — dropping _isReady = false wouldn't fail this test.

@mkzung mkzung Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

}

[Test]
public override void ResetsProperly()
{
Expand Down
Loading