aminghadersohi commented on PR #37200:
URL: https://github.com/apache/superset/pull/37200#issuecomment-3761600817
## Review Feedback Addressed
Thank you for the thorough code review! I've pushed a commit that addresses
all the feedback:
### Changes Made:
1. **OOM Protection for Token Estimation** (CodeAnt suggestion)
- Wrapped `estimate_response_tokens()` call in try/except to handle
`MemoryError`
- Added fallback to treat estimation failures as over-limit (safe default)
- Pass `None` instead of the full `response` to
`format_size_limit_error()` to avoid double serialization of large responses
2. **Specific Exception Types** (Bito suggestions)
- `middleware.py:862` - Changed from `Exception` to `(ValueError,
TypeError, AttributeError)` for event logger
- `middleware.py:918` - Changed from `Exception` to `(ImportError,
AttributeError, KeyError)` for factory function
- `token_utils.py:86` - Changed from `Exception` to `(TypeError,
ValueError, AttributeError)`
- `token_utils.py:117` - Changed from `Exception` to `(TypeError,
ValueError, AttributeError)`
- `token_utils.py:301` - Changed from `Exception` to `(TypeError,
ValueError, KeyError, AttributeError)`
3. **Documentation Fix** (Bito suggestion)
- Fixed comment that said default `token_limit` is 50,000 to correctly
say 25,000
- Fixed in both `mcp_config.py` and `middleware.py` docstring
### Note on Test File Context Manager Syntax
The parenthesized context manager syntax `with (patch(...), patch(...)):` is
**valid Python 3.10+ syntax** and Superset requires Python 3.10+. The `ruff`
formatter prefers this style over line continuations. The CodeAnt suggestions
about this being a tuple bug are incorrect for Python 3.10+ projects.
All pre-commit checks pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
