Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

👌 IMPROVE: Terminal states do not reference process #205

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions plumpy/base/state_machine.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,11 @@ class State:
def is_terminal(cls) -> bool:
return not cls.ALLOWED

def __init__(self, state_machine: 'StateMachine', *args: Any, **kwargs: Any): # pylint: disable=unused-argument
def __init__(self, state_machine: Optional['StateMachine'], *args: Any, **kwargs: Any): # pylint: disable=unused-argument
"""
:param state_machine: The process this state belongs to
"""
self.state_machine = state_machine
self.state_machine: Optional[StateMachine] = state_machine
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like a ValueError might be useful to be riased if state_machine is None for non terminal states. What do you think?

If it mattered more I would almost change the class hierarchy to have a TerminalState which doesn't even take it as an argument and is hardcoded to return False on is_terminal but I don't think it's worth it in this case but the exception might be useful. Just because I don't know how hardcoded the assumption is that the state_machine returns a non-None.

self.in_state: bool = False

def __str__(self) -> str:
Expand Down Expand Up @@ -157,6 +157,8 @@ def exit(self) -> None:
raise InvalidStateError('Cannot exit a terminal state {}'.format(self.LABEL))

def create_state(self, state_label: Hashable, *args: Any, **kwargs: Any) -> 'State':
if self.state_machine is None:
raise ValueError('State machine not set for {self}')
return self.state_machine.create_state(state_label, *args, **kwargs)

def do_enter(self) -> None:
Expand Down
17 changes: 11 additions & 6 deletions plumpy/process_states.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ def process(self) -> state_machine.StateMachine:
"""
:return: The process
"""
if self.state_machine is None:
raise ValueError('Process not set for {self}')
return self.state_machine

def load_instance_state(self, saved_state: SAVED_STATE_TYPE, load_context: persistence.LoadSaveContext) -> None:
Expand Down Expand Up @@ -342,14 +344,17 @@ class Excepted(State):
TRACEBACK = 'traceback'

def __init__(
self, process: 'Process', exception: Optional[BaseException], trace_back: Optional[TracebackType] = None
self,
process: 'Process', # pylint: disable=unused-argument
exception: Optional[BaseException],
trace_back: Optional[TracebackType] = None
):
"""
:param process: The associated process
:param exception: The exception instance
:param trace_back: An optional exception traceback
"""
super().__init__(process)
super().__init__(None) # terminal state does not require process ref
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you just elaborate on this comment a litle to say
'Terminal states don't get a backreference to the state machine so that it's easier for python to clear the memory'
or something to that effect just so that it's clear:

  1. Why this was done, and,
  2. So it's clear under which circumstances it's None (which you've already stated actually)

self.exception = exception
self.traceback = trace_back

Expand Down Expand Up @@ -389,8 +394,8 @@ def get_exc_info(self) -> Tuple[Optional[Type[BaseException]], Optional[BaseExce
class Finished(State):
LABEL = ProcessState.FINISHED

def __init__(self, process: 'Process', result: Any, successful: bool) -> None:
super().__init__(process)
def __init__(self, process: 'Process', result: Any, successful: bool) -> None: # pylint: disable=unused-argument
super().__init__(None) # terminal state does not require process ref
self.result = result
self.successful = successful

Expand All @@ -399,13 +404,13 @@ def __init__(self, process: 'Process', result: Any, successful: bool) -> None:
class Killed(State):
LABEL = ProcessState.KILLED

def __init__(self, process: 'Process', msg: Optional[str]):
def __init__(self, process: 'Process', msg: Optional[str]): # pylint: disable=unused-argument
"""
:param process: The associated process
:param msg: Optional kill message

"""
super().__init__(process)
super().__init__(None) # terminal state does not require process ref
self.msg = msg


Expand Down
2 changes: 1 addition & 1 deletion plumpy/version.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# -*- coding: utf-8 -*-
__version__: str = '0.18.4'
__version__: str = '0.18.5a0'

__all__ = ['__version__']