fix(cli): avoid DetachedInstanceError in compute-thumbnails#41530
Conversation
Code Review Agent Run #d49e9fActionable 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 |
|
| Language | Fuzzy before | Fuzzy after | New |
|---|---|---|---|
ar |
1130 | 1133 | +3 |
it |
1660 | 1662 | +2 |
ko |
1514 | 1516 | +2 |
pt |
1835 | 1837 | +2 |
How to fix
1. Install dependencies (if not already set up):
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext2. Re-extract strings and sync .po files:
./scripts/translations/babel_update.shThis rewrites superset/translations/messages.pot from the current source files and merges the changes into every .po file. Strings whose msgid changed will be marked #, fuzzy.
3. Resolve the fuzzy entries in the affected language files (ar, it, ko, pt):
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.poFor each fuzzy entry, either rewrite the msgstr to match the new string and remove the #, fuzzy line, or clear the msgstr to "" if you cannot provide a translation.
4. Commit your changes to the .po files.
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| items = [(model.id, str(model)) for model in query.all()] | ||
| count = len(items) | ||
| if asynchronous: | ||
| func = compute_func.delay | ||
| action = "Triggering" | ||
| else: | ||
| func = compute_func | ||
| action = "Processing" |
There was a problem hiding this comment.
Suggestion: Add explicit type annotations for the newly introduced local variables to comply with the type-hinting requirement. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The added local variables items, count, func, and action are all inferred from context and can be explicitly annotated, so this matches the type-hinting rule for newly introduced Python code without type hints.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/cli/thumbnails.py
**Line:** 88:95
**Comment:**
*Custom Rule: Add explicit type annotations for the newly introduced local variables to comply with the type-hinting requirement.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The suggestion to add explicit type annotations is correct and improves code readability and maintainability. You can resolve this by adding the following type hints to the variables in items: list[tuple[int, str]] = [(model.id, str(model)) for model in query.all()]
count: int = len(items)
if asynchronous:
func: Callable = compute_func.delay
action: str = "Triggering"
else:
func: Callable = compute_func
action: str = "Processing"There are no other comments on this PR. Would you like me to check for any other potential type-hinting improvements in this file? superset/cli/thumbnails.py |
|
@rusackas as requested |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41530 +/- ##
==========================================
- Coverage 64.57% 64.56% -0.01%
==========================================
Files 2718 2718
Lines 151528 151529 +1
Branches 34819 34819
==========================================
- Hits 97845 97839 -6
- Misses 51854 51858 +4
- Partials 1829 1832 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| def __init__(self, pk: int, label: str) -> None: | ||
| self.id = pk | ||
| self._label = label | ||
| self.detached = False |
There was a problem hiding this comment.
Suggestion: Add explicit type annotations for the instance attributes in the initializer so the new class fully complies with the type-hinting rule. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The new class defines instance attributes in Python without annotations even though their types are clear and can be annotated. This matches the type-hinting rule for relevant variables in newly introduced Python code.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/cli/thumbnails_test.py
**Line:** 31:34
**Comment:**
*Custom Rule: Add explicit type annotations for the instance attributes in the initializer so the new class fully complies with the type-hinting rule.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #4614c3Actionable 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 |
…thumbnails_test.py
fddd08d to
9441669
Compare
| func = compute_func | ||
| action = "Processing" | ||
| for i, (model_pk, label) in enumerate(items): | ||
| msg = f'{action} {friendly_type} "{label}" ({i + 1}/{count})' |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this newly introduced local variable to comply with the rule requiring type hints on relevant annotatable variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly introduced local variable that can clearly be annotated as str, but it is assigned without a type hint. That matches the rule requiring type hints on relevant annotatable variables.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/cli/thumbnails.py
**Line:** 100:100
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced local variable to comply with the rule requiring type hints on relevant annotatable variables.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| compute loop, so building the progress message never touches a detached | ||
| instance even when several models are processed. | ||
| """ | ||
| models = [_FakeModel(1, "Dashboard A"), _FakeModel(2, "Dashboard B")] |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this local collection variable to satisfy the type-hinting requirement for relevant variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The new code introduces a local collection variable without an explicit type hint, and the rule requires type hints on relevant Python variables that can be annotated. This is a real omission in the added code.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/cli/thumbnails_test.py
**Line:** 52:52
**Comment:**
*Custom Rule: Add an explicit type annotation for this local collection variable to satisfy the type-hinting requirement for relevant variables.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| """ | ||
| models = [_FakeModel(1, "Dashboard A"), _FakeModel(2, "Dashboard B")] | ||
|
|
||
| query = mocker.MagicMock() |
There was a problem hiding this comment.
Suggestion: Add a type annotation to this mock variable so the new code does not omit type hints on relevant local variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The mock variable is newly introduced and has no explicit type hint. Since the rule flags relevant variables that can be annotated, this is a valid type-hint omission.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/cli/thumbnails_test.py
**Line:** 54:54
**Comment:**
*Custom Rule: Add a type annotation to this mock variable so the new code does not omit type hints on relevant local variables.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| side_effect=_detach_session, | ||
| ) | ||
|
|
||
| result = CliRunner().invoke(thumbnails.compute_thumbnails, ["-d"]) |
There was a problem hiding this comment.
Suggestion: Add a type annotation for this command invocation result variable to comply with the type-hint rule for relevant variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The invocation result is stored in a new local variable without a type hint. Under the stated rule, this is a valid omission because the variable can be annotated.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/cli/thumbnails_test.py
**Line:** 70:70
**Comment:**
*Custom Rule: Add a type annotation for this command invocation result variable to comply with the type-hint rule for relevant variables.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Code Review Agent Run #c60ad8Actionable 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 |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Thanks @carlosribeiro-plaid. Clean fix and the test covers the detach mid-loop rather than just asserting the refactor. LGTM.
The codeant nitpicks about missing type hints on msg and the test's local vars aren't worth blocking on, those are all trivially inferred by mypy already.
rusackas
added
the
merge-if-green
|
Bito Automatic Review Skipped – PR Already Merged |
3 participants
SUMMARY
superset compute-thumbnailscrashes withDetachedInstanceErrorwhen more than one model is processed (e.g.superset compute-thumbnails -d -i 8 -i 12).compute_generic_thumbnailqueries the models, then inside the loop builds the progress message with an f-string that interpolates the ORM instance (f'... "{model}" ...'). Computing a thumbnail (cache_dashboard_thumbnail/cache_chart_thumbnail) can close/expire the SQLAlchemy session, so on the next iteration the instance is detached andstr(model)raises:The fix materializes the
(id, label)pairs up front, before any thumbnail is computed, then iterates over plain values — so the loop never touches a detached ORM instance. No behavior change otherwise.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (CLI bug fix).
TESTING INSTRUCTIONS
superset compute-thumbnails -d -i <id1> -i <id2>(two or more ids).DetachedInstanceError.ADDITIONAL INFORMATION