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

Set self.state_machine to None for terminal states #198

Open
chrisjsewell opened this issue Jan 28, 2021 · 1 comment · May be fixed by #205
Open

Set self.state_machine to None for terminal states #198

chrisjsewell opened this issue Jan 28, 2021 · 1 comment · May be fixed by #205

Comments

@chrisjsewell
Copy link
Member

The next issue is with the plumpy state, which is cyclic since the Process keeps a reference of the state and the state keeps a reference of the Process:

In [19]: from plumpy.process_states import State
In [20]: states = [o for o in all_objects if hasattr(o, "__class__") and isinstance(o, State)] 
    ...: states                                                                                                                   
Out[20]: [<plumpy.process_states.Finished at 0x7f56a01f5ac0>]

In [21]: f = states[0]                                                                                                            

In [22]: f.__dict__                                                                                                               
Out[22]: 
{'state_machine': <aiida_sleep.sleep_job.SleepCalculation at 0x7f56a04f55e0>,
 'in_state': True,
 'result': ExitCode(status=0, message=None, invalidates_cache=False),
 'successful': True,
 '_called': 0,
 'state': None}

In [23]: f.state_machine._state
Out[23]: <plumpy.process_states.Finished at 0x7f56a01f5ac0>

Originally posted by @chrisjsewell in aiidateam/aiida-core#4603 (comment)

@chrisjsewell
Copy link
Member Author

Additional conversation:

@muhrin

Regarding the state-process circular ref. Yes, I see, although I'm a bit surprised the garbage collector doesn't deal with this (after all for non-circular references you can get away with no GC and just rely on reference counting). In any case, it doesn't feel like an ideal solution to set the state to None (although I'm not opposed to it if there is no relatively easy alternative). Do you think a weakref from the State to the Process might help the GC? Even this isn't ideal as if a client gets a state reference but let's the process go out of scope they would find things to be broken.

@chrisjsewell

I think this is more trouble than it's worth 😬 As you say, it could lead to non-determistic errors with the Process going out of scope.

I don't really see any other good solution apart from setting the state to None, but open to suggestion.
Note the type checking should now pick up anywhere this is an issue, i.e. where a null value has to be accounted for

@muhrin

Yes, I think you're probably right. Ok, how 'bout just doing what you say and putting a check in the state property getter to raise an error with a messages pinpointing what has happened. Hopefully it never gets hit anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant