fix(sql): don't classify objects with permissive __getattr__ as connections#10296
Closed
Jayashanker-Padishala wants to merge 2 commits into
Closed
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
…ctions Instantiating any pytorch-ignite Metric hung the executing cell forever: ignite metrics implement __getattr__ to return a lazy MetricsLambda for any attribute name (with a traceback.format_stack call on each access), so they pass every getattr-based duck-typing check. The post-execution data-source hook then classified the metric as an ADBC connection and adbc_get_objects()/read_all() walked into the lazy object graph and never returned real data. Probe one attribute that no real connection defines; if the object fabricates a callable for it, reject it in AdbcDBAPIEngine.is_compatible and DBAPIEngine.is_compatible. Genuine connection proxies that forward __getattr__ to an underlying connection are unaffected, since the probe does not exist there either. Fixes marimo-team#10213
Jayashanker-Padishala
force-pushed
the
fix-10213-permissive-getattr-sql-sniffing
branch
from
July 23, 2026 10:35
bc88018 to
2ef4c5f
Compare
Author
|
I have read the CLA Document and I hereby sign the CLA |
Member
|
Thanks @Jayashanker-Padishala! I made a similar fix here with some additions #10301. But thank you for checking, appreciate it. |
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
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.
This pull request was authored by a coding agent. (Per AGENTS.md: authored by Claude Code under the direction of @Jayashanker-Padishala, who takes responsibility for it. Marked as draft per policy.)
Fixes #10213
📝 Summary
Instantiating any
ignite.metrics.Metrichung the executing cell forever. Root cause (found via faulthandler stack dump of the hung kernel):__getattr__to return a lazyMetricsLambdafactory for any attribute name — and each access runstraceback.format_stack()(ignitemetric.py:797), so every attribute probe is also slow.callable(getattr(var, method, None))duck-typing check succeed, so the post-execution_broadcast_data_source_connectionhook classified the metric as an ADBC connection.AdbcConnectionCatalog.get_databases→adbc_get_objects(...).read_all().to_pylist()then walked into the lazy object graph and never produced real data — the cell appears hung and is not interruptible.🔧 Fix
Add
fabricates_attributes()(in_sql/engines/types.py): probe one attribute that no real connection defines; if the object returns something for it, its duck-typing answers are meaningless andis_compatiblerejects it. Applied to bothAdbcDBAPIEngineandDBAPIEngine(the two engines relying on puregetattrduck-typing — the others use isinstance checks).Deliberately not
inspect.getattr_static: genuine connection-pool proxies legitimately forward__getattr__to an underlying connection and would regress; they pass the probe (the probe attribute doesn't exist on the real connection either).This generalizes the existing ibis
Deferredspecial-case (#7791) — any permissive-__getattr__object (lazy proxies, mocks) is now handled, not just known names.🧪 Verification
executing_kernelfixture +from ignite.metrics import Loss; metric = Loss(nn.MSELoss())): watchdog fired at 30s pre-fix; post-fix the same cell completes in ~4s withmetricdefined.test_is_compatible_rejects_fabricated_attributes(dbapi),test_adbc_is_compatible_rejects_fabricated_attributes(adbc),test_fabricated_attributes_not_treated_as_engine(get_engines).pytest tests/_sql/: 246 passed.ruff check+ruff format --checkclean on all changed files.