diff --git a/python/flink_agents/plan/actions/action.py b/python/flink_agents/plan/actions/action.py index c3ad31457..339753e1c 100644 --- a/python/flink_agents/plan/actions/action.py +++ b/python/flink_agents/plan/actions/action.py @@ -48,7 +48,6 @@ class Action(BaseModel): model_config = ConfigDict(arbitrary_types_allowed=True) name: str - # TODO: Raise a warning when the action has a return value, as it will be ignored. exec: PythonFunction | JavaFunction trigger_conditions: List[str] config: Dict[str, Any] | None = None @@ -116,3 +115,4 @@ def __init__( ) # TODO: Update expected signature after import State and Context. self.exec.check_signature(Event, RunnerContext) + self.exec.warn_if_returns_value(self.name) diff --git a/python/flink_agents/plan/function.py b/python/flink_agents/plan/function.py index 28bc5089b..7988084ae 100644 --- a/python/flink_agents/plan/function.py +++ b/python/flink_agents/plan/function.py @@ -108,6 +108,14 @@ class Function(BaseModel, ABC): def check_signature(self, *args: Tuple[Any, ...]) -> None: """Check function signature is legal or not.""" + def warn_if_returns_value(self, action_name: str) -> None: + """Warn if this function declares a non-``None`` return annotation. + + Action return values are discarded by the framework, so declaring a + return type is almost always a mistake. Only Python callables can be + introspected, so non-Python function types treat this as a no-op. + """ + @abstractmethod def __call__(self, *args: Tuple[Any, ...], **kwargs: Dict[str, Any]) -> Any: """Execute function.""" @@ -191,6 +199,24 @@ def check_signature(self, *args: Tuple[Any, ...]) -> None: except TypeError as e: raise TypeError(err_msg) from e + def warn_if_returns_value(self, action_name: str) -> None: + """Warn that an action's return value is ignored by the framework. + + Actions communicate by sending events via ``ctx.send_event(...)``; any + value they return is discarded. A non-``None`` return annotation is + almost always a mistake, so surface it when the plan is built. The + stringized ``"None"`` is accepted for modules that opt into + ``from __future__ import annotations``. + """ + return_annotation = inspect.signature(self.__get_func()).return_annotation + if return_annotation not in (inspect.Signature.empty, None, type(None), "None"): + logger.warning( + f"Action '{action_name}' declares return type " + f"'{return_annotation}', but action return values are ignored by " + f"the framework. Actions should send events via " + f"ctx.send_event(...) and return None." + ) + def __call__(self, *args: Tuple[Any, ...], **kwargs: Dict[str, Any]) -> Any: """Execute the stored function with provided arguments. diff --git a/python/flink_agents/plan/tests/test_action.py b/python/flink_agents/plan/tests/test_action.py index f93ee4531..7ed692d1a 100644 --- a/python/flink_agents/plan/tests/test_action.py +++ b/python/flink_agents/plan/tests/test_action.py @@ -16,6 +16,7 @@ # limitations under the License. ################################################################################# import json +import logging from pathlib import Path import pytest @@ -36,6 +37,10 @@ def illegal_signature(value: int, ctx: RunnerContext) -> None: pass +def returns_value(event: Event, ctx: RunnerContext) -> str: + return "ignored by the framework" + + def test_action_signature_legal() -> None: Action( name="legal", @@ -53,6 +58,37 @@ def test_action_signature_illegal() -> None: ) +def test_action_warns_when_returns_value( + caplog: pytest.LogCaptureFixture, +) -> None: + with caplog.at_level(logging.WARNING): + Action( + name="returns_value", + exec=PythonFunction.from_callable(returns_value), + trigger_conditions=[InputEvent.EVENT_TYPE], + ) + assert any( + "returns_value" in record.getMessage() + and "ignored" in record.getMessage().lower() + for record in caplog.records + ) + + +def test_action_no_warning_when_returns_none( + caplog: pytest.LogCaptureFixture, +) -> None: + with caplog.at_level(logging.WARNING): + Action( + name="legal", + exec=PythonFunction.from_callable(legal_signature), + trigger_conditions=[InputEvent.EVENT_TYPE], + ) + assert not any( + "ignored by the framework" in record.getMessage() + for record in caplog.records + ) + + @pytest.fixture(scope="module") def action() -> Action: func = PythonFunction.from_callable(legal_signature) diff --git a/python/flink_agents/plan/tests/test_agent_plan.py b/python/flink_agents/plan/tests/test_agent_plan.py index 04fdb6c79..c92e36b0e 100644 --- a/python/flink_agents/plan/tests/test_agent_plan.py +++ b/python/flink_agents/plan/tests/test_agent_plan.py @@ -16,6 +16,7 @@ # limitations under the License. ################################################################################# import json +import logging from pathlib import Path from typing import Any, ClassVar, Dict, List, Sequence @@ -565,6 +566,31 @@ def test_add_action_and_resource_to_agent() -> None: assert actual == expected +def _returns_value_action(event: Event, ctx: RunnerContext) -> str: + return "ignored by the framework" + + +def test_warns_for_returning_action_added_via_add_action( + caplog: pytest.LogCaptureFixture, +) -> None: + """Actions registered imperatively via add_action are also checked, since + the warning now lives in Action.__init__ rather than a single collection path. + """ + agent = Agent() + agent.add_action( + name="returns_value", + trigger_conditions=[InputEvent.EVENT_TYPE], + func=_returns_value_action, + ) + with caplog.at_level(logging.WARNING): + AgentPlan.from_agent(agent, AgentConfiguration()) + assert any( + "returns_value" in record.getMessage() + and "ignored" in record.getMessage().lower() + for record in caplog.records + ) + + # ── String identifier tests ──────────────────────────────────────────────