Tmp am/raas#93
Conversation
radical.edge changed the notification pattern from a per-session _notify closure to a _plugin reference with _dispatch_notify(). Update all callsites in RoseSession to match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Four new tests in TestRoseSession: - test_submit_workflow_dispatches_submitted_notification: checks that submit_workflow fires workflow_state/SUBMITTED via _dispatch_notify - test_notify_state_calls_dispatch_notify: checks _notify_state payload - test_notify_state_no_plugin_does_not_raise: guards against AttributeError when _plugin is None (bare unit-test creation without a plugin) - test_task_event_dispatched_via_plugin: verifies the _on_done callback in _run_workflow fires a task_event notification Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the notification system to use a plugin-based dispatch mechanism, replacing the previous direct callback approach. The updates span workflow submission, task event handling, and state notifications, with corresponding unit tests added to verify the new behavior. Review feedback highlights opportunities to reduce redundancy by reusing the _notify_state method and to improve consistency by using wf.to_dict() for notification payloads.
| if self._plugin: | ||
| self._plugin._dispatch_notify( | ||
| "workflow_state", | ||
| {"wf_id": wf_id, "state": "SUBMITTED", "workflow_file": workflow_file}, | ||
| ) |
There was a problem hiding this comment.
The notification logic here is redundant with the _notify_state method. Reusing self._notify_state(wf) would simplify the code, ensure consistency in the notification payload, and centralize the logic for dispatching workflow state updates.
self._notify_state(wf)| if self._plugin: | ||
| self._plugin._dispatch_notify( | ||
| "workflow_state", | ||
| {"wf_id": wf.wf_id, "state": wf.state.value, "stats": wf.stats, "error": wf.error}, | ||
| {"wf_id": wf.wf_id, "state": wf.state.value, | ||
| "stats": wf.stats, "error": wf.error}, | ||
| ) |
There was a problem hiding this comment.
Instead of manually constructing a dictionary with a subset of fields, consider using wf.to_dict(). This ensures that the notification payload is consistent with the status API and includes useful metadata like start_time, end_time, and workflow_file. Additionally, note that _dispatch_notify is a protected method of the plugin; if the base Plugin class provides a public notification API, it should be preferred to maintain proper encapsulation.
if self._plugin:
self._plugin._dispatch_notify("workflow_state", wf.to_dict())
|
@andre-merzky LGTM. Thanks |
2 participants
notification updates.