Use tp_getattro for AttributeError suggestions instead of the __getattr__ hook#126
Merged
jhonabreul merged 2 commits intoJul 2, 2026
Conversation
Reimplement the AttributeError member-name suggestions on top of a ClassObject.tp_getattro override, replacing the miss-only __getattr__ hook (AttributeErrorHint) that was merged in QuantConnect#124. The hook installed a shared __getattr__ on every reflected type and manually rewired tp_getattro to CPython's slot_tp_getattr_hook. That surgery is significantly more invasive for no measurable real-world benefit: the per-access cost it avoids (~17 ns) is lost in the noise on realistic Lean workloads. This version keeps the enrichment entirely in a tp_getattro override that delegates to PyObject_GenericGetAttr and only does work on a miss, and drops all the slot manipulation. Behaviour and messages are unchanged (snake_case "Did you mean ...?" suggestions); the existing Python and embedding tests are untouched and still pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Martin-Molinero
approved these changes
Jul 2, 2026
This was referenced Jul 2, 2026
jhonabreul
added a commit
that referenced
this pull request
Jul 6, 2026
…e re-land of the miss-only hook) (#129) * Revert to the miss-only __getattr__ hook for AttributeError suggestions Revert the code changes of #126, restoring the AttributeErrorHint miss-only __getattr__ hook approach from #124: successful attribute accesses go straight through CPython's native generic getattr with no managed transition; only a miss enters managed code to build the "Did you mean ...?" suggestions. The package <Version> is left at 2.0.56 (not reverted). Note: as restored here, the hook still has the off-GIL callback defect that crashed Lean's CI on 2.0.55 (the __clr_attr_msg__ delegate is invoked through MethodBinder, which releases the GIL); it is fixed in the follow-up commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Fix off-GIL AttributeError-hint callback: use a native __getattr__ thunk The __getattr__ hook restored from #124 exposed the hint builder to Python as a .NET delegate (Func<PyObject, string, string>). Python invoked it through DelegateObject.tp_call -> MethodBinder.Invoke, and MethodBinder releases the GIL around every reflected invocation (allow_threads defaults to true). The callback therefore ran CPython C-API calls (GetPythonType, GetManagedString, ...) without holding the GIL on every attribute miss (hasattr, getattr with default, typos). It usually survived by luck, but segfaulted whenever the pythonnet Finalizer fired mid-callback: Finalizer.DisposeAll() starts with PyErr_Fetch, which dereferences the current thread state - NULL when the GIL is not held. This is what crashed Lean's CI test host on 2.0.55 after all 35k tests passed. Replace the Python-function-plus-delegate pair with a native method descriptor: a PyMethodDef (METH_VARARGS) around a managed thunk, turned into __getattr__ via PyDescr_NewMethod (newly bound). CPython's slot_tp_getattr_hook now calls the managed hook directly as a native method call with the GIL held - MethodBinder is never involved. The PyMethodDef and thunk are allocated once and kept for the process lifetime, since descriptors reference them and can outlive engine shutdown bookkeeping. Verified with an instrumented build (PyGILState_Check inside BuildMissingAttributeMessage): the delegate-based hook reports "GIL held: False" on every miss; this version reports "GIL held: True". Also survives a 20s stress run of concurrent attribute misses plus finalizer churn across 6 threads, and behaves identically for hasattr/ getattr-with-default, dunder probes, Python subclasses with their own __getattr__, and suggestion messages. Add a regression test asserting the installed __getattr__ is a native method_descriptor, which is the property that keeps the callback out of MethodBinder's allow-threads path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Reimplements the
AttributeErrormember-name suggestions (added in #124, v2.0.55) on top of aClassObject.tp_getattrooverride, and removes the miss-only__getattr__hook. Behavior is unchanged — a missing attribute on a .NET object still gets a snake_caseDid you mean ...?hint:Version bumped to 2.0.56.
Why replace the hook
#124 installed a shared
__getattr__on every reflected type and manually rewiredtp_getattroto CPython's privateslot_tp_getattr_hook(address scraped from a probe class). That is a lot of low-level CPython surgery on the type system, done for one reason: to keep the successful attribute-access ("hit") path free of the small per-access cost that atp_getattrooverride adds.Benchmarking showed that cost is not worth the complexity:
tp_getattrooverride is ~17–19 ns/access.Given the negligible real-world impact, this PR drops the slot manipulation entirely and keeps the enrichment in a straightforward
tp_getattrothat delegates toPyObject_GenericGetAttrand only does work on a miss.Diff vs master
DynamicClassObjectcontinues to enrich on its own attribute-miss path (unchanged). Suggestions are ranked by case-insensitive Levenshtein distance + substring containment, dunder names are skipped, and member names are emitted in snake_case (const/static-readonly →UPPER_CASE) via the existingToSnakeCasehelpers.Testing
tests/test_class.py) and embedding tests (src/embed_tests/TestPropertyAccess.cs) — unchanged from Suggest similar member names on AttributeError for .NET objects #124 and passing.QuantConnect.TestsPython/Pandas unit suite and allPython/*regression algorithms were previously run against this feature with zero regressions vs master (all failures pre-existing/environmental).🤖 Generated with Claude Code