chore: add paths to backend extension stack traces#37300
chore: add paths to backend extension stack traces#37300villebro merged 1 commit intoapache:masterfrom
Conversation
villebro
requested review from
eschutho,
geido,
kgabryje,
michael-s-molina and
rusackas
as code owners
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #b474c8Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| source_base_path: ( | ||
| str # Base path for traceback filenames (absolute path or supx:// URL) | ||
| ) |
There was a problem hiding this comment.
Suggestion: Breaking change: adding source_base_path as a required dataclass field (no default) will cause existing code that constructs LoadedExtension without this new argument to raise a TypeError at runtime. Make the field optional or provide a default to preserve backward compatibility. [possible bug]
Severity Level: Critical 🚨
- ❌ Extension loading fails with TypeError during instantiation.
- ❌ Background tasks using extensions fail at startup/runtime.
- ⚠️ Backwards compatibility for third‑party extensions broken.| source_base_path: ( | |
| str # Base path for traceback filenames (absolute path or supx:// URL) | |
| ) | |
| source_base_path: str = "" # Base path for traceback filenames (absolute path or supx:// URL) |
Steps of Reproduction ✅
1. Open a Python REPL or run any process that imports the dataclass: `from
superset.extensions.types import LoadedExtension` (file:
`superset/extensions/types.py:30-39`).
2. Instantiate the dataclass with the pre-PR argument set used across the codebase, e.g.:
`LoadedExtension(id="ext1", name="ext", manifest=..., frontend={}, backend={},
version="1.0")`
(this mirrors how LoadedExtension objects were constructed before adding the new
field).
3. Observe Python raising a TypeError similar to:
"TypeError: __init__() missing 1 required positional argument: 'source_base_path'"
This is raised from the dataclass-generated __init__ defined by `LoadedExtension` (see
`superset/extensions/types.py:30-39`).
4. Runtime effect: any code path that previously constructed LoadedExtension without the
new argument (e.g. extension registration/loader) will fail at instantiation time. Because
the PR description and stacktrace examples reference extension execution, this will
manifest when extensions are loaded or registered during startup or when tasks execute
(see `superset/tasks/executor.py:108` in PR description).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/extensions/types.py
**Line:** 37:39
**Comment:**
*Possible Bug: Breaking change: adding `source_base_path` as a required dataclass field (no default) will cause existing code that constructs `LoadedExtension` without this new argument to raise a TypeError at runtime. Make the field optional or provide a default to preserve backward compatibility.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment.
Not an issue, as this feature is still in development
|
CodeAnt AI finished reviewing your PR. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #37300 +/- ##
===========================================
+ Coverage 0 67.60% +67.60%
===========================================
Files 0 644 +644
Lines 0 48309 +48309
Branches 0 5298 +5298
===========================================
+ Hits 0 32661 +32661
- Misses 0 14357 +14357
- Partials 0 1291 +1291
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
SUMMARY
Currently when backend extensions raise exceptions, they don't contain a reference to the actual file (notice
File: "<string>"at the top of the stack):This makes it difficult to track down where the error actually occurred.
In this PR, we propose stack traces will have a reference to the source file as follows:
supxextensions, the reference will besupx://<extension-id>/file.py. This URL-like format is due to the fact that the python file doesn't exist physically on the system (it's inside the bundle)..../dist/.../file.py.This will make stack traces more actionable, both when developing locally, and also when using bundles.
Local extension after changes:
Notice that for local extensions, it's now showing the actual code
x = 120/0that raised the error to help narrow down the problematic code. This is thanks to the interpreter being able to read the file on the filesystem.supx bundle after changes:
Here we're intentionally not showing the offending code, as we don't want to expose more of the internals than necessary (line number and method should be fine). We can add parity with local extensions for bundles, but IMO it's an unnecessary leak of extension internals, so I advise against it. It would also consume slightly more memory, as we'd have to load the supx code into linecache. While the memory overhead is arguably negligible, it's still unnecessary IMO. But I'm curious to hear how others feel about this.
ADDITIONAL INFORMATION