Closed
Conversation
Member
Author
|
@sourcery-ai dismiss |
Contributor
Reviewer's GuideIntroduces a more powerful matcher/DI system with Depends/DependsFactory, event priority handling and fallback preset events, adds Anthropic adapter and CoT-think tag filtering, and wires these into ChatObject with preset fallback retries, while updating docs, config, protocol, and tests. Class diagram for matcher, dependency injection, and fallback eventsclassDiagram
class EventTypeEnum {
<<enum>>
PRECOMPLETION
COMPLETION
Nil
BEFORE_COMPLETION
PRESET_FALLBACK
}
class BaseEvent {
<<abstract>>
+event_type EventTypeEnum
+get_event_type() EventTypeEnum
}
class FallbackContext {
+preset ModelPreset
+exc_info BaseException
+config AmritaConfig
+context SendMessageWrap
+term int
+fail(reason Any) Never
+get_event_type() EventTypeEnum
}
class FunctionData {
+function Callable
+signature inspect.Signature
+frame FrameType
+priority int
+matcher Matcher
}
class EventRegistry {
-_event_handlers defaultdict~str,defaultdict~int,list~FunctionData~~~~
+register_handler(event_type str, data FunctionData) void
+get_handlers(event_type str) defaultdict~int,list~FunctionData~~
+get_all() defaultdict~str,defaultdict~int,list~FunctionData~~~~
}
class Matcher {
+event_type str
+priority int
+block bool
+append_handler(func Callable[...,Awaitable~Any~]]) void
+set_block(block bool) void
+stop_process() void
+cancel_matcher() void
+pass_event() void
}
class DependsFactory~T~ {
-_depency_func Callable[...,T|Awaitable~T~]
+resolve(*args, **kwargs) T|None
}
class MatcherFactory {
+trigger_event(event BaseEvent, config AmritaConfig, *args, **kwargs) void
+_resolve_dependencies(signature inspect.Signature, session_args Iterable~Any~, session_kwargs dict~str,Any~) (bool,tuple,dict~str,Any~,dict~str,DependsFactory~Any~~)
+_do_runtime_resolve(runtime_args dict~int,DependsFactory~Any~~, runtime_kwargs dict~str,DependsFactory~Any~~, args2update list~Any~, kwargs2update dict~str,Any~, session_args list~Any~, session_kwargs dict~str,Any~, exception_ignored tuple~type~BaseException~~) bool
+_simple_run(matcher_list list~FunctionData~, event BaseEvent, config AmritaConfig, exception_ignored tuple~type~BaseException~~, extra_args tuple, extra_kwargs dict~str,Any~) bool
}
class MatcherManager {
}
class AmritaConfig {
}
class ModelPreset {
+name str
+model str
+api_key str
+base_url str
+config ModelConfig
}
class ModelConfig {
+stream bool
+top_p float
+temperature float
+cot_model bool
}
class LLMConfig {
+auto_retry bool
+max_retries int
+max_fallbacks int
}
class ChatObject {
+preset ModelPreset
+config AmritaConfig
+context_wrap SendMessageWrap
+_response_queue asyncio.Queue~RESPONSE_TYPE~
+_overflow_queue asyncio.Queue~RESPONSE_TYPE~
+_process_chat() None
}
class FallbackFailed {
<<exception>>
}
class MatcherException {
<<exception>>
}
class CancelException {
<<exception>>
}
class BlockException {
<<exception>>
}
class PassException {
<<exception>>
}
class ChatException {
<<typealias>>
}
BaseEvent <|-- FallbackContext
FallbackContext ..> ModelPreset
FallbackContext ..> AmritaConfig
FallbackContext ..> SendMessageWrap
FallbackContext ..> FallbackFailed
MatcherException <|-- CancelException
MatcherException <|-- BlockException
MatcherException <|-- PassException
ChatException <.. MatcherException
Matcher "1" o-- "*" FunctionData : owns
FunctionData --> Matcher : matcher
EventRegistry "1" --> "*" FunctionData : stores
MatcherFactory ..> EventRegistry
MatcherFactory ..> DependsFactory
MatcherFactory ..> FunctionData
MatcherFactory ..> BaseEvent
MatcherFactory ..> AmritaConfig
MatcherManager <|-- MatcherFactory
ChatObject ..> MatcherManager : trigger_event
ChatObject ..> FallbackContext
ChatObject ..> LLMConfig
EventTypeEnum <.. BaseEvent
EventTypeEnum <.. FallbackContext
FallbackFailed <|-- RuntimeError
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
builtins/adapter.py, the helpermodel_dumpreuses the nameobjfor both the iterable and the loop variable (for obj in obj), which will fail at runtime; rename the loop variable (e.g.for item in obj) and useitem.model_dump()accordingly. - In
MatcherFactory._simple_run, the checkisinstance(e, CancelException | BlockException)will never match becauseCancelException | BlockExceptionis atypes.UnionType; use a tuple instead, e.g.isinstance(e, (CancelException, BlockException)), so cancel/block behavior works as intended. - In
ChatObject._process_chat, the fallback warning message logs({i}/{self.config.llm.max_retries})while the loop is controlled bymax_fallbacks, which is misleading; update the message to reflectmax_fallbacksor the actual controlling value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `builtins/adapter.py`, the helper `model_dump` reuses the name `obj` for both the iterable and the loop variable (`for obj in obj`), which will fail at runtime; rename the loop variable (e.g. `for item in obj`) and use `item.model_dump()` accordingly.
- In `MatcherFactory._simple_run`, the check `isinstance(e, CancelException | BlockException)` will never match because `CancelException | BlockException` is a `types.UnionType`; use a tuple instead, e.g. `isinstance(e, (CancelException, BlockException))`, so cancel/block behavior works as intended.
- In `ChatObject._process_chat`, the fallback warning message logs `({i}/{self.config.llm.max_retries})` while the loop is controlled by `max_fallbacks`, which is misleading; update the message to reflect `max_fallbacks` or the actual controlling value.
## Individual Comments
### Comment 1
<location path="src/amrita_core/chatmanager.py" line_range="840-841" />
<code_context>
+ logger.warning(
+ f"Because of `{e!s}`, LLM request failed, retrying ({i}/{self.config.llm.max_retries})..."
+ )
+ ctx = FallbackContext(self.preset, e, self.config, self.context_wrap, i)
+ await MatcherManager.trigger_event(ctx, ctx.config, (FallbackFailed,))
+ if ctx.preset is self.preset:
+ ctx.fail("No preset fallback available, exiting!")
</code_context>
<issue_to_address>
**issue (bug_risk):** The `FallbackFailed` exception type is not actually passed to `exception_ignored` in `trigger_event`.
Here `(FallbackFailed,)` is passed as a positional arg and is treated as an extra DI parameter, so `exception_ignored` remains its default `()`. That means `FallbackFailed` from a fallback matcher is caught and logged instead of re-raised. To ensure it’s ignored by `_simple_run`, call:
```python
await MatcherManager.trigger_event(
ctx,
ctx.config,
exception_ignored=(FallbackFailed,),
)
```
</issue_to_address>
### Comment 2
<location path="src/amrita_core/hook/matcher.py" line_range="180-189" />
<code_context>
+ return rs
+
+
+def Depends(dependency: Callable[..., T | Awaitable[T]]) -> Any:
+ """
+ Dependency injection decorator.
+
+ Args:
+ dependency: The dependency function to inject
+
+ Returns:
+ DependsFactory: A factory for dependency injection
+
+ Example:
+ ```python
+ async def get_example_dependency(...) -> Any | None:
+ ...
+
+ @on_precompletion()
+ async def a_function_with_dependencies(
+ event: PreCompletionEvent,
+ dep: ExampleDependency = Depends(get_example_dependency),
+ ):
+ ...
+ ```
+
+ If DependendsFactory's return is None, this function won't be called.
+ """
+ return DependsFactory[T](dependency)
+
+
</code_context>
<issue_to_address>
**suggestion:** The `Depends` helper returns `Any` and the generic argument is unused, which weakens typing for dependency injection.
Because the function returns `Any`, callers lose the information that `DependsFactory` is parameterized by `T`, so type checkers can’t infer the injected dependency type. Returning the concrete generic instead would preserve typing, e.g.:
```python
def Depends(dependency: Callable[..., T | Awaitable[T]]) -> DependsFactory[T]:
return DependsFactory(dependency)
```
That way, handler parameters like `dep: Example = Depends(get_example)` will have the correct inferred type.
Suggested implementation:
```python
def Depends(dependency: Callable[..., T | Awaitable[T]]) -> DependsFactory[T]:
```
```python
return DependsFactory(dependency)
```
If `Any` was only used as the return type of `Depends`, you should also:
1. Remove `Any` from the `from typing import ...` list in this file to avoid an unused import.
</issue_to_address>
### Comment 3
<location path="tests/test_matcher.py" line_range="307" />
<code_context>
+ assert result is False
+ assert kwargs2update == {} # Should not be updated
+
+ @pytest.mark.asyncio
+ async def test_do_runtime_resolve_exception(self):
+ """Test _do_runtime_resolve with exception in dependency."""
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for `_do_runtime_resolve` where an exception type listed in `exception_ignored` is raised, to verify the special handling path.
You can mirror `test_do_runtime_resolve_exception` but use a custom exception passed in `exception_ignored` and assert it is raised directly, e.g.:
```python
@pytest.mark.asyncio
async def test_do_runtime_resolve_exception_ignored(self):
class CustomError(Exception):
pass
async def error_dep() -> str:
raise CustomError("ignored")
factory = DependsFactory(error_dep)
with pytest.raises(CustomError):
await MatcherFactory._do_runtime_resolve(
{}, # runtime_args
{"key": factory}, # runtime_kwargs
[], # args2update
{}, # kwargs2update
[], # session_args
{}, # session_kwargs
(CustomError,), # exception_ignored
)
```
This verifies that ignored exceptions are re-raised instead of being collected into the `ExceptionGroup`.
```suggestion
"""Test _do_runtime_resolve with exception in dependency."""
@pytest.mark.asyncio
async def test_do_runtime_resolve_exception_ignored(self):
class CustomError(Exception):
pass
async def error_dep() -> str:
raise CustomError("ignored")
factory = DependsFactory(error_dep)
with pytest.raises(CustomError):
await MatcherFactory._do_runtime_resolve(
{}, # runtime_args
{"key": factory}, # runtime_kwargs
[], # args2update
{}, # kwargs2update
[], # session_args
{}, # session_kwargs
(CustomError,), # exception_ignored
)
```
</issue_to_address>
### Comment 4
<location path="docs/guide/api-reference/classes/DependsFactory.md" line_range="8" />
<code_context>
+## Constructor
+
+```python
+def __init__(self, depency: Callable[..., T | Awaitable[T]])
+```
+
</code_context>
<issue_to_address>
**issue (typo):** Parameter name `depency` looks like a typo and should likely be `dependency`.
Since this appears in user-facing API docs, updating the parameter name there as well will prevent confusion and keep terminology consistent with the rest of the text.
Suggested implementation:
```
def __init__(self, dependency: Callable[..., T | Awaitable[T]])
```
```
- `dependency`: The dependency function to wrap, which can return any type `T` or an awaitable object `Awaitable[T]`
```
</issue_to_address>
### Comment 5
<location path="docs/zh/guide/api-reference/classes/DependsFactory.md" line_range="8" />
<code_context>
+## Constructor
+
+```python
+def __init__(self, depency: Callable[..., T | Awaitable[T]])
+```
+
</code_context>
<issue_to_address>
**issue (typo):** Constructor parameter `depency` appears to be a misspelling of `dependency`.
建议在文档和代码中统一改为 `dependency`，以避免歧义并保持一致性。
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+840
to
+841
| ctx = FallbackContext(self.preset, e, self.config, self.context_wrap, i) | ||
| await MatcherManager.trigger_event(ctx, ctx.config, (FallbackFailed,)) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The FallbackFailed exception type is not actually passed to exception_ignored in trigger_event.
Here (FallbackFailed,) is passed as a positional arg and is treated as an extra DI parameter, so exception_ignored remains its default (). That means FallbackFailed from a fallback matcher is caught and logged instead of re-raised. To ensure it’s ignored by _simple_run, call:
await MatcherManager.trigger_event(
ctx,
ctx.config,
exception_ignored=(FallbackFailed,),
)
Comment on lines
+180
to
+189
| def Depends(dependency: Callable[..., T | Awaitable[T]]) -> Any: | ||
| """ | ||
| Dependency injection decorator. | ||
|
|
||
| Args: | ||
| dependency: The dependency function to inject | ||
|
|
||
| Returns: | ||
| DependsFactory: A factory for dependency injection | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion: The Depends helper returns Any and the generic argument is unused, which weakens typing for dependency injection.
Because the function returns Any, callers lose the information that DependsFactory is parameterized by T, so type checkers can’t infer the injected dependency type. Returning the concrete generic instead would preserve typing, e.g.:
def Depends(dependency: Callable[..., T | Awaitable[T]]) -> DependsFactory[T]:
return DependsFactory(dependency)That way, handler parameters like dep: Example = Depends(get_example) will have the correct inferred type.
Suggested implementation:
def Depends(dependency: Callable[..., T | Awaitable[T]]) -> DependsFactory[T]: return DependsFactory(dependency)If Any was only used as the return type of Depends, you should also:
- Remove
Anyfrom thefrom typing import ...list in this file to avoid an unused import.
|
|
||
| @pytest.mark.asyncio | ||
| async def test_do_runtime_resolve_exception(self): | ||
| """Test _do_runtime_resolve with exception in dependency.""" |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for _do_runtime_resolve where an exception type listed in exception_ignored is raised, to verify the special handling path.
You can mirror test_do_runtime_resolve_exception but use a custom exception passed in exception_ignored and assert it is raised directly, e.g.:
@pytest.mark.asyncio
async def test_do_runtime_resolve_exception_ignored(self):
class CustomError(Exception):
pass
async def error_dep() -> str:
raise CustomError("ignored")
factory = DependsFactory(error_dep)
with pytest.raises(CustomError):
await MatcherFactory._do_runtime_resolve(
{}, # runtime_args
{"key": factory}, # runtime_kwargs
[], # args2update
{}, # kwargs2update
[], # session_args
{}, # session_kwargs
(CustomError,), # exception_ignored
)This verifies that ignored exceptions are re-raised instead of being collected into the ExceptionGroup.
Suggested change
| """Test _do_runtime_resolve with exception in dependency.""" | |
| """Test _do_runtime_resolve with exception in dependency.""" | |
| @pytest.mark.asyncio | |
| async def test_do_runtime_resolve_exception_ignored(self): | |
| class CustomError(Exception): | |
| pass | |
| async def error_dep() -> str: | |
| raise CustomError("ignored") | |
| factory = DependsFactory(error_dep) | |
| with pytest.raises(CustomError): | |
| await MatcherFactory._do_runtime_resolve( | |
| {}, # runtime_args | |
| {"key": factory}, # runtime_kwargs | |
| [], # args2update | |
| {}, # kwargs2update | |
| [], # session_args | |
| {}, # session_kwargs | |
| (CustomError,), # exception_ignored | |
| ) |
| ## Constructor | ||
|
|
||
| ```python | ||
| def __init__(self, depency: Callable[..., T | Awaitable[T]]) |
Contributor
There was a problem hiding this comment.
issue (typo): Parameter name depency looks like a typo and should likely be dependency.
Since this appears in user-facing API docs, updating the parameter name there as well will prevent confusion and keep terminology consistent with the rest of the text.
Suggested implementation:
def __init__(self, dependency: Callable[..., T | Awaitable[T]])
- `dependency`: The dependency function to wrap, which can return any type `T` or an awaitable object `Awaitable[T]`
| ## Constructor | ||
|
|
||
| ```python | ||
| def __init__(self, depency: Callable[..., T | Awaitable[T]]) |
Contributor
There was a problem hiding this comment.
issue (typo): Constructor parameter depency appears to be a misspelling of dependency.
建议在文档和代码中统一改为 dependency，以避免歧义并保持一致性。
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
1 participant
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.
close #22
Summary by Sourcery
Introduce a new, priority-aware matcher and dependency injection system for event handling, add preset fallback events and Anthropic adapter support, and extend chat and configuration flows to support CoT filtering and robust LLM fallback behavior.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: