No-op message_handler (and sibling) decorators for abstract methods

[nissa-seru]

Why are these changes needed?

@message_handler and sibling decorators are processed at time of declaration, including checking whether the type hints on decorated methods are valid (since they are used by the runtime for determining which type of message the method will handle.) This creates a catch-22 when declaring abstract message handlers:

1. The abstract method can omit the @message_handler decorator, but this (justifiably) causes typechecking errors when adding the decorator to implementations of the abstract method (in subclasses) due to the implementation being decorated and the abstract method being undecorated

2. The abstract method can have the @message_handler decorator and use a concrete type; this causes typechecking errors on implementations if they handle a different concrete type (of the flavor Type parameter "ReceivesT@MessageHandler" is invariant, but "TextPayload" is not the same as "AbstractPayload")

3. The abstract method can have the @message_handler decorator and use a more appropriate typehint (ie a TypeVar) in the signature of the abstract method. This satisfies the typechecker, but currently raises at declaration time because the decorator is processing the abstract method's signature for validity as an instantiable method handler (even though it can never be directly instantiated due to being an abstract method.)

I'm hopeful that in future we can explore how to make the type-hint-based routing mechanism more flexible, as ReceivesT being invariant results in poor ergonomics for message handlers (for example, I would like to be able to use generics in message handler type hints.) However, I'm not yet familiar enough with the code to confidently judge how much of this inconvenience is due to a knowledge gap on my end (either a specific gap, or just natural learning curve) versus an actual cause for change.

Accordingly, this PR opts for the conservative band-aid of making the message_handler decorators no-op if the decorated method is abstract. This unblocks option 3 (as described above) to enable use of ABC with message handlers without requiring typechecking suppression on each implementation.

Checks

• I've included any doc changes needed for https://microsoft.github.io/autogen/. See https://github.com/microsoft/autogen/blob/main/CONTRIBUTING.md to build and test documentation locally.
• I have not added any documentation for this; I believe the change is minor and sufficiently inline with expected behavior such that it rests much lower down the totem pole than other helpful details that could be added to the current docs.
• I've added tests (if relevant) corresponding to the changes introduced in this PR.
• I've confirmed that the change unblocks option 3 (as described above) at runtime, but have not added any tests to this effect. One option could be to test that the exclusion logic is not overbroad, but this feels somewhat recreational to me given the specificity.
• I've made sure all auto checks have passed.
• poe check passes on my local clone main with Fix poe check on Windows #5942 merged. Without Fix poe check on Windows #5942, a variety of tests will fail on Windows for reasons detailed in that PR (and unrelated to this one.)

[ekzhu]

Thanks for the PR! Could you try to remove this type ignore here and see if the type checking is happy:

• https://github.com/microsoft/autogen/blob/main/python/packages/autogen-agentchat/src/autogen_agentchat/teams/_group_chat/_magentic_one/_magentic_one_orchestrator.py#L125-L126
• https://github.com/microsoft/autogen/blob/main/python/packages/autogen-agentchat/src/autogen_agentchat/teams/_group_chat/_magentic_one/_magentic_one_orchestrator.py#L183

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.77%. Comparing base (296de52) to head (1b32de4).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5945      +/-   ##
==========================================
- Coverage   75.77%   75.77%   -0.01%     
==========================================
  Files         191      191              
  Lines       13100    13100              
==========================================
- Hits         9927     9926       -1     
- Misses       3173     3174       +1     

Flag	Coverage Δ
unittests	75.77% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nissa-seru]

@ekzhu Ah, sorry, I think my note may have been unclear. This PR, by itself, won't make the typechecker happy anywhere that it wasn't before. The only impact it has is to prevent runtime errors from @message_handler (and sibling) decorations on abstract methods that would otherwise cause a runtime error due to their type signature being flagged as an invalid type for message-handling purposes.

It does enable a solution to the instances you mention - you could decorate with @abstractmethod the overridden methods in the parent class BaseGroupChatManager and give them more permissive typing using, say, a TypeVar, since this PR would prevent doing so from raising a runtime error. However, I see that the relevant methods in BaseGroupChatManager are not currently abstract (even though the class as whole is) - and I'm not familiar enough with the AgentChat code + implementors of BaseGroupChatManager to know if there would be any knockon effects from making them abstract.

The upshot being - this PR will not resolve any typechecking errors on the lines you linked, such that I could just remove the #type: ignore comments. You could modify BaseGroupChatManager if desired (as noted above), but I'll leave that up to you given the interfaces.

[ekzhu]

okay I think I get it. This PR allows for abstract message handlers. I think this is useful especially considering we want to support using Core RoutedAgent as participant in AgentChat teams. Having an abstract base class for participant and group chat manager with abstract message handlers can be helpful to enforce the contract.

I will leave this to @jackgerrits to decide. It's not urgent to merge this now but let's come back to this next week.

[a-holm]

I found a potential issue. @nissa-seru The goal of making the decorators no-op for abstract methods to support type variables in signatures is a good one.

However, I believe the current implementation doesn't quite achieve the intended effect. The getattr(func, "__isabstractmethod__", False) check is added to the outer dispatch logic, but it doesn't prevent the inner decorator function (which contains the type hint validation logic) from being called with the abstract method itself. This means the problematic validation (get_type_hints, get_types) will likely still run during class definition time, potentially raising the same errors the PR aims to prevent.

To achieve the no-op behavior, the check for __isabstractmethod__ should likely be placed inside the decorator inner function. If the decorated function is abstract, the decorator function should return the original function func directly, bypassing the validation and wrapping logic. I havent tested it, but it looks like it and should probably be considered.