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

Workflow: Centralize working directory #745

Merged
merged 10 commits into from
Jun 28, 2023

Conversation

liamhuber
Copy link
Member

Moves the working directory up to the IsNodal mix-in class. In the process I had to bring up the parent attribute too; Node.parent works as before, but here Node.working_directory is allowed to exist even without a parent. Workflow.parent now exists but conforms to the existing spec that workflows can't have parents -- i.e. it's forced to only ever be None and still doesn't appear as an __init__ arg.

If we want to change the node spec this way, this PR closes #744.

@liamhuber liamhuber requested a review from samwaseda June 28, 2023 04:21
@github-actions
Copy link
Contributor

Binder 👈 Launch a binder notebook on branch pyiron/pyiron_contrib/centralize_working_directory

@liamhuber liamhuber mentioned this pull request Jun 28, 2023
pyiron_contrib/workflow/workflow.py Outdated Show resolved Hide resolved
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self._parent = None

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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

@liamhuber liamhuber changed the title Centralize working directory Workflow: Centralize working directory Jun 28, 2023
@liamhuber liamhuber merged commit 471ed49 into is_nodal_mixin Jun 28, 2023
@delete-merged-branch delete-merged-branch bot deleted the centralize_working_directory branch June 28, 2023 17:10
@samwaseda
Copy link
Member

uhm.... I didn't approve it yet...

@samwaseda
Copy link
Member

but ok now since self._parent got clarified I'm fine.

@liamhuber
Copy link
Member Author

uhm.... I didn't approve it yet...

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.

but ok now since self._parent got clarified I'm fine.

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.

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 this pull request may close these issues.

None yet

2 participants