Open ligerzero-ai opened 1 week ago
ligerzero-ai
commented
1 week ago Here I'd like to have a generic try; except block because potcar generation CAN fail for a multitude of reasons, not just because structure is not provided, which would be easy to catch. In that case, I'd like ot raise a custom error that is easily traceable
liamhuber
commented
1 week ago @ligerzero-ai, thanks for the question!
Your MWE is missing some data for me to reproduce it, but I can already provide some hints.
First, at a high level, the node you defined is illegal. A Function node should not have internal structure (i.e. no internal subgraph); from context I'm inferring get_default_POTCAR_paths is another node you've written.
There's a couple routes you can go here. The first is to simply leverage the function you are trying to get, as you are trying to get it, by directly accessing the underlying function on the node like: potcar_paths = get_default_POTCAR_paths.node_function(structure, pp_set = pp_set). The other would be to break your current node down into smaller pieces and then re-assemble them as a Macro.
Otherwise raising a custom error from inside a function node works just like you do here. Once you resolve the function node-inside a-function node issue, the custom exception will run fine:
from pyiron_workflow import Workflow
@Workflow.wrap.as_function_node()
def MyNode(mandatory, optional=None):
class MyCustomError(ValueError):
pass
if optional == mandatory:
raise MyCustomError("Anything but that!")
return 42
n = MyNode("mandatory", "mandatory")
n.run()yields
---------------------------------------------------------------------------
MyCustomError Traceback (most recent call last)
Cell In[44], line 14
11 return 42
13 n = MyNode("mandatory", "mandatory")
---> 14 n.run()
File ~/work/pyiron/pyiron_workflow/pyiron_workflow/node.py:495, in Node.run(self, run_data_tree, run_parent_trees_too, fetch_input, check_readiness, force_local_execution, emit_ran_signal, *args, **kwargs)
492 if self.parent is not None:
493 self.parent.register_child_starting(self)
--> 495 return super().run(
496 check_readiness=check_readiness,
497 force_local_execution=force_local_execution,
498 _finished_callback=(
499 self._finish_run_and_emit_ran if emit_ran_signal else self._finish_run
500 ),
501 )
File ~/work/pyiron/pyiron_workflow/pyiron_workflow/run.py:151, in Runnable.run(self, check_readiness, force_local_execution, _finished_callback)
149 if check_readiness and not self.ready:
150 raise ReadinessError(self._readiness_error_message)
--> 151 return self._run(
152 finished_callback=(
153 self._finish_run if _finished_callback is None else _finished_callback
154 ),
155 force_local_execution=force_local_execution,
156 )
File ~/work/pyiron/pyiron_workflow/pyiron_workflow/run.py:34, in manage_status.<locals>.wrapped_method(runnable, *args, **kwargs)
32 runnable.failed = True
33 out = None
---> 34 raise e
35 finally:
36 # Leave the status as running if the method returns a future
37 runnable.running = isinstance(out, Future)
File ~/work/pyiron/pyiron_workflow/pyiron_workflow/run.py:29, in manage_status.<locals>.wrapped_method(runnable, *args, **kwargs)
27 runnable.running = True
28 try:
---> 29 out = status_managed_method(runnable, *args, **kwargs)
30 return out
31 except Exception as e:
File ~/work/pyiron/pyiron_workflow/pyiron_workflow/run.py:179, in Runnable._run(self, finished_callback, force_local_execution)
173 raise ValueError(
174 f"{self.label} got 'self' as a run kwarg, but self is already the "
175 f"first positional argument passed to :meth:`on_run`."
176 )
177 if force_local_execution or self.executor is None:
178 # Run locally
--> 179 run_output = self.on_run(*args, **kwargs)
180 return finished_callback(run_output)
181 else:
182 # Just blindly try to execute -- as we nail down the executor interaction
183 # we'll want to fail more cleanly here.
File ~/work/pyiron/pyiron_workflow/pyiron_workflow/function.py:313, in Function.on_run(self, **kwargs)
312 def on_run(self, **kwargs):
--> 313 return self.node_function(**kwargs)
Cell In[44], line 9, in MyNode(mandatory, optional)
6 pass
8 if optional == mandatory:
----> 9 raise MyCustomError("Anything but that!")
11 return 42
MyCustomError: Anything but that!I haven't managed to cut out the middle of the stack trace, but we start and end the trace by stating the custom error so I think it's not too bad.
ligerzero-ai
commented
1 week ago Thanks for the quick feedback - here's the full structure:
from pyiron_workflow.function import as_function_node
@as_function_node("potcar_path_set")
def get_default_POTCAR_paths(structure, pseudo_lib_path = None, pseudo_set = "PBE.54"):
site_element_list = [site.species_string for site in structure]
past_element = site_element_list[0]
element_list = [past_element]
for element in site_element_list:
if element != past_element:
element_list.append(element)
past_element = element
print(element_list)
potcar_paths = []
for element in element_list:
potcar_paths.append(os.path.join(pseudo_lib_path, element, "POTCAR"))
return potcar_paths
@as_function_node("potcar_path")
def write_POTCAR(directory, potcar_paths=None, structure=None, filename="POTCAR", pp_set = "PBE.54"):
class PotcarNotGeneratedError(Exception):
"""Exception raised when the POTCAR file is not found."""
pass
if potcar_paths == None:
try:
potcar_paths = get_default_POTCAR_paths(structure, pp_set = pp_set)
except Exception as e:
print("You must specify a structure if you are using default generation!")
raise PotcarNotGeneratedError
potcar_path = os.path.join(directory, filename)
with open(os.path.join(directory, "POTCAR"),'wb') as wfd:
for f in potcar_paths:
with open(f,'rb') as fd:
shutil.copyfileobj(fd, wfd)
return potcar_pathCan I ask; what makes it illegal? Am I not allowed to call a node from within a node? So basically inputs cannot be generated from within the node using another node?
ligerzero-ai
commented
1 week ago Then should I just define the get_default_POTCAR_paths as a traditional function?
liamhuber
commented
1 week ago Can I ask; what makes it illegal?
"Illegal" was probably too strong -- "not recommended" is probably better.
Whatever you're putting in the decorated function is getting invoked at every .run call; so if you hide other nodes inside there, they're quite ephemeral -- they get instantiated, do their thing (if you run them) and then are lost. Because they are full nodes you get full node behaviour -- readiness checks, the need to actually .run them, etc.
In your case, potcar_paths = get_default_POTCAR_paths(structure, pp_set = pp_set) is actually making potcar_paths a node instance and not running. It then tries to run later at for f in potcar_paths: -- what's actually happening here is that I don't have a clean __enter__ dunder set up, but it is resulting in a GetItem node getting injected; I guess when this tries to get data from upstream potcar_paths realizes it's not "ready".
So you can stick nodes inside a function node, but it comes a some computational cost and might get you hard-to-parse errors.
Here's a toy example of it working:
from pyiron_workflow import Workflow
@Workflow.wrap.as_function_node("y")
def SomeOtherNode(x: int):
return x + 1
@Workflow.wrap.as_function_node()
def MyNode(mandatory, optional=None):
class MyCustomError(ValueError):
pass
if optional == mandatory:
raise MyCustomError("Anything but that!")
else:
from_an_ephemeral_node = SomeOtherNode(mandatory).run()
return from_an_ephemeral_node * 2
n = MyNode(1, 2)
n.run()
>>> 4But don't do it 😝
Then should I just define the
get_default_POTCAR_pathsas a traditional function?
Yes, if you don't have any reason to use it directly as a node elsewhere, it's totally fine to define it as a function and use it inside the function node.
It is early days to be strict about "best practices", but I also think it's A-OK to internally reference the other node's function ala potcar_paths = get_default_POTCAR_paths.node_function(structure, pp_set = pp_set). In principle you can think about turning (acyclic) macro nodes into function nodes by using the same trick. In the future we may actually do this on purpose as a way of sort of "compiling" workflows into raw python function calls.
here's the full structure:
When making nodes, I like to frustrate my IDE's linting and define them using PascalCase as though they were classes -- because that's what the decorator is turning them into, e.g.:
@as_function_node("potcar_path_set")
def GetDefaultPOTCARPaths(structure, pseudo_lib_path = None, pseudo_set = "PBE.54"):
I would also suggest restructuring your input parsing a little to follow a pattern like:
if potcar_paths is None:
if structure is None:
raise ValueError("At least one of potcar_paths or structure must be specified!"
else:
potcar_paths = get_default_POTCAR_paths.node_function(structure, pp_set = pp_set)
IMO the error when they're both None is then a bit more informative, and you don't risk swallowing or obfuscating the error if something actually goes wrong with get_default_POTCAR_paths.
EDIT: got rid of some "..." in the code snippets that we messing with the formatting
ligerzero-ai
commented
1 week ago Cheers Liam, thanks for your very thorough comments! The first versions of a vasp workflow with some queue submission functionality should be online in the next few weeks. Sorry about the delay - manuscripts are always more work than I think they are.
I've been spending some time thinking about how queue submission should work. If the goal is to have individual vasp-execution nodes be sent off to the queue, is the only way for the workflow to work via a cron-like/user triggered node-status pull, carried out via a convergence check on each executing folder that exists on the graph? Maybe the workflow is run again, until it bumps into the unresolved parts of the graph?
Let us assume that the user can't have a permanent kernel running in the background (since we are aiming for broad applicability here to many users).
liamhuber
commented
1 week ago That sounds great! I'm on mobile now but let's open a separate issue/chat live regarding queue submission. Very briefly my plan was to serialize the nose and leverage pysqa and a script that deserializes the node, runs it, and reserializes it. Then when the (restarted) workflow runs into a node that is running it checks to see if the finished serialized object is there and loads it and continues the graph if it is.
Yields: