Message passing with more information #291

unkcpz · 2024-12-01T11:30:16Z

Simplipy _create_state_instance so it only need to do real object creation.
Killed state message is a message type
Make message not global variable and able to attach with more information.
pause/play/state all using build method to build the message.

unkcpz · 2024-12-01T11:31:54Z

A lot of single quotes to double quotes changes came from my IDE linter, should be reduced if #289 get merged and this PR will contains much less line changes.

codecov · 2024-12-01T21:13:37Z

Codecov Report

Attention: Patch coverage is 92.13483% with 7 lines in your changes missing coverage. Please review.

Project coverage is 90.90%. Comparing base (14b7c1a) to head (b1d9159).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
src/plumpy/processes.py	84.00%	4 Missing ⚠️
src/plumpy/base/state_machine.py	86.67%	2 Missing ⚠️
src/plumpy/process_comms.py	97.37%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
+ Coverage   90.77%   90.90%   +0.13%     
==========================================
  Files          22       22              
  Lines        2990     2999       +9     
==========================================
+ Hits         2714     2726      +12     
+ Misses        276      273       -3

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

unkcpz · 2024-12-02T08:52:51Z

src/plumpy/base/state_machine.py

+    def _create_state_instance(self, state_cls: type[State], **kwargs: Any) -> State:
+        if state_cls.LABEL not in self.get_states_map():
+            raise ValueError(f'{state_cls.LABEL} is not a valid state')


This is one of the major changes in this PR. I made this function only create the instance as name indicated. If it is already a state instance the logic is moved directly to transition_to method of state machine.

unkcpz · 2024-12-02T08:54:40Z

src/plumpy/process_comms.py

+class PlayMessage:
+    @classmethod
+    def build(cls, message: str | None = None) -> MessageType:
+        return {
+            INTENT_KEY: Intent.PLAY,
+            MESSAGE_KEY: message,
+        }
+
+
+class PauseMessage:
+    @classmethod
+    def build(cls, message: str | None = None) -> MessageType:
+        return {
+            INTENT_KEY: Intent.PAUSE,
+            MESSAGE_KEY: message,
+        }
+
+
+class KillMessage:
+    @classmethod
+    def build(cls, message: str | None = None, force: bool = False) -> MessageType:
+        return {
+            INTENT_KEY: Intent.KILL,
+            MESSAGE_KEY: message,
+            FORCE_KILL_KEY: force,
+        }
+
+
+class StatusMessage:
+    @classmethod
+    def build(cls, message: str | None = None) -> MessageType:
+        return {
+            INTENT_KEY: Intent.STATUS,
+            MESSAGE_KEY: message,
+        }
+


Instead of using global variable for message and using copy.copy to create message for different embedded message content, I use the class with build method for message build.

unkcpz · 2024-12-02T08:57:46Z

src/plumpy/base/state_machine.py

@@ -300,16 +314,28 @@ def _fire_state_event(self, hook: Hashable, state: Optional[State]) -> None:
    def on_terminated(self) -> None:
        """Called when a terminal state is entered"""

-    def transition_to(self, new_state: Union[Hashable, State, Type[State]], *args: Any, **kwargs: Any) -> None:
+    def transition_to(self, new_state: State | type[State] | None, **kwargs: Any) -> None:


Build new_state with Hashable and from State class are the same, I made the use of transition_to more consistent by allow only passing the State class or an existed state.

I was wrong about this, using Hashable key to get the state type is more generic, it allows to redefine the state class dynamically for the specific state machine by overriding the STATES_MAP. I'll revert changes back.

unkcpz · 2024-12-02T09:00:10Z

src/plumpy/base/state_machine.py

+        if new_state is None:
+            return None
+


This is to cover the case in Process.step when the state.execute lead to the next state is none from terminal state.

unkcpz · 2024-12-03T09:58:40Z

src/plumpy/processes.py

+        state_class = self.get_states_map()[process_states.ProcessState.KILLED]
+        new_state = self._create_state_instance(state_class, msg=msg)
+        self.transition_to(new_state)


Compare to the one liner of original implementation, the new one seems more verbose. The state object needs to be created from the state tag with the parameters directly and passed to the transition_to. But, in my opinion, this API is a bit readable. The transition_to now only need to deal with the case where the passed in state is an object, instead of decide whether needs to create the state instance case by case.

unkcpz marked this pull request as draft December 1, 2024 11:30

This was referenced Dec 1, 2024

Type extension aiopika workaround #292

Closed

WIP: Implement option to force kill #288

Open

unkcpz force-pushed the Message-passing-with-more-information branch 2 times, most recently from ed2a043 to 96c5842 Compare December 1, 2024 21:12

unkcpz force-pushed the Message-passing-with-more-information branch from 43e1520 to 737adf3 Compare December 1, 2024 23:56

unkcpz added pr/blocked PR is blocked by another PR that should be merged first and removed pr/blocked PR is blocked by another PR that should be merged first labels Dec 2, 2024

unkcpz force-pushed the Message-passing-with-more-information branch from 737adf3 to 1b2864c Compare December 2, 2024 08:34

unkcpz added 10 commits December 2, 2024 09:35

amend from rebase

1117eeb

Add default MESSAGE_KEY to None value and FORCE_KILL_KEY

b82791d

Alias MessageType for message passing

d4c0489

Simplify _create_state_instance so it only need to do real create

c5a195c

Furthur simplipy _create_state_instant only create state from class

8db6675

Killed state all through passing msg

74d048d

Amend

667af7a

If transition_to None do noting

4be6931

KillMessage build msg from parameters

88259d6

Pause/Play/Status all using message builder

c3c9db4

unkcpz force-pushed the Message-passing-with-more-information branch 3 times, most recently from 2ffdab2 to d0e4e73 Compare December 2, 2024 08:41

Remove duplicate codes

d0e4e73

unkcpz marked this pull request as ready for review December 2, 2024 08:41

unkcpz force-pushed the Message-passing-with-more-information branch from b218122 to e3c2ae8 Compare December 2, 2024 08:50

Future type annotation

e3c2ae8

unkcpz commented Dec 2, 2024

View reviewed changes

unkcpz requested a review from khsrali December 2, 2024 09:17

unkcpz requested a review from sphuber December 2, 2024 09:18

unkcpz force-pushed the Message-passing-with-more-information branch from b1d9159 to e5c74ad Compare December 2, 2024 12:53

Fix doc

e5c74ad

unkcpz marked this pull request as draft December 3, 2024 00:49

unkcpz force-pushed the Message-passing-with-more-information branch from 7c105bd to 17a541a Compare December 3, 2024 09:53

Mapping states from state name

17a541a

unkcpz commented Dec 3, 2024

View reviewed changes

unkcpz marked this pull request as ready for review December 3, 2024 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Message passing with more information #291

Message passing with more information #291

unkcpz commented Dec 1, 2024 •

edited

Loading

unkcpz commented Dec 1, 2024

codecov bot commented Dec 1, 2024 •

edited

Loading

unkcpz Dec 2, 2024

unkcpz Dec 2, 2024

unkcpz Dec 2, 2024

unkcpz Dec 3, 2024

unkcpz Dec 2, 2024

unkcpz Dec 3, 2024

Message passing with more information #291

Are you sure you want to change the base?

Message passing with more information #291

Conversation

unkcpz commented Dec 1, 2024 • edited Loading

unkcpz commented Dec 1, 2024

codecov bot commented Dec 1, 2024 • edited Loading

Codecov Report

unkcpz Dec 2, 2024

Choose a reason for hiding this comment

unkcpz Dec 2, 2024

Choose a reason for hiding this comment

unkcpz Dec 2, 2024

Choose a reason for hiding this comment

unkcpz Dec 3, 2024

Choose a reason for hiding this comment

unkcpz Dec 2, 2024

Choose a reason for hiding this comment

unkcpz Dec 3, 2024

Choose a reason for hiding this comment

unkcpz commented Dec 1, 2024 •

edited

Loading

codecov bot commented Dec 1, 2024 •

edited

Loading