-
Notifications
You must be signed in to change notification settings - Fork 11
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
Workflow: Centralize working directory #745
Conversation
…ir own working directory without a parent
That workflow's can't have parents.
That nodes can have working directories
pyiron_contrib/workflow/workflow.py
Outdated
@@ -121,17 +120,12 @@ class Workflow(IsNodal, HasToDict, HasNodes): | |||
wrap_as = _NodeDecoratorAccess | |||
|
|||
def __init__(self, label: str, *nodes: Node, strict_naming=True): | |||
self._parent = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self._parent = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need this or the IDE complains later about defining an attribute outside init
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, i need to double check on a real computer but I think your point is this is done in super and you're right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I remember now. IsNodal
doesn't use the property/setter obfuscation -- it just directly applies the init kwarg self.parent = parent
. To conform to the spec that workflows shouldn't have parents, I wrap this by having an underlying self._parent
attribute whose setter raises an error if you set it with anything other than None
. Thus, we need to declare the private variable before calling super.__init__
since the super call will exploit the setter.
Since parent
isn't exposed in Workflow.__init__
and defaults to None
in IsNodal.__init__
, this should all be redundant. But it keeps it all working together while ensuring that someone gets an error if they ever try to set wf.parent = foo
.
In the future, if we want to change the spec so workflows can have parents, all we need to do is get rid of the private var/setter/getter protection here (and maybe expose parent as a kwarg) and we could directly lean on the how super class handles the variable. I.e. it's really easy to unwind these protections if we change our mind about what the behaviour ought to be. There's a comment to this effect in the setter, but I'll expand it a bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But self._parent
is not used anywhere else other than this line right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But
self._parent
is not used anywhere else other than this line right?
Ah, after committing your other suggestion its not 🤦♂️
I added your suggestion over in #729
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah the two suggestions were meant to be connected. I guess there's no way on GitHub to make one suggestion with two separate lines
Co-authored-by: Sam Dareska <[email protected]>
uhm.... I didn't approve it yet... |
but ok now since |
No worries, I'm happy to open a new PR with further changes you want. In the cases where you request explicit and significant changes that I disagree with, I'll hold off until we reach consensus. But if there's just a few comments and they're either small or I incorporate your feedback directly, then I'll probably be a bit pragmatic/hasty and merge them. I often have hours every day to work on this, but that doesn't start until ~18:00 your time. It's not reasonable to expect you to keep reviewing and providing feedback consistently that late in the evening and I often want to keep building on stuff without worrying about huge merge headaches later, so I'd rather push it through and then make new PRs to fix anything you hate afterwards.
Super! Yeah, this is what I'm talking about -- if the potential disagreement looks small and simple enough I'll probably just go for it and fix things later. |
Moves the working directory up to the
IsNodal
mix-in class. In the process I had to bring up theparent
attribute too;Node.parent
works as before, but hereNode.working_directory
is allowed to exist even without aparent
.Workflow.parent
now exists but conforms to the existing spec that workflows can't have parents -- i.e. it's forced to only ever beNone
and still doesn't appear as an__init__
arg.If we want to change the node spec this way, this PR closes #744.