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

Registration by identifier alone #156

Merged
merged 15 commits into from
Jan 15, 2024
Merged
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
2 changes: 1 addition & 1 deletion docs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ But the intent is to collect them together into a workflow and leverage existing

```python
>>> from pyiron_workflow import Workflow
>>> Workflow.register("plotting", "pyiron_workflow.node_library.plotting")
>>> Workflow.register("pyiron_workflow.node_library.plotting", "plotting")
>>>
>>> @Workflow.wrap_as.single_value_node()
... def Arange(n: int):
Expand Down
4 changes: 2 additions & 2 deletions notebooks/atomistics_nodes.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
"metadata": {},
"outputs": [],
"source": [
"Workflow.register(\"atomistics\", \"pyiron_workflow.node_library.atomistics\")\n",
"Workflow.register(\"plotting\", \"pyiron_workflow.node_library.plotting\")"
"Workflow.register(\"pyiron_workflow.node_library.atomistics\", \"atomistics\")\n",
"Workflow.register(\"pyiron_workflow.node_library.plotting\", \"plotting\")"
]
},
{
Expand Down
4 changes: 2 additions & 2 deletions notebooks/deepdive.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -1664,8 +1664,8 @@
}
],
"source": [
"wf.register(\"pyiron_atomistics\", \"pyiron_workflow.node_library.pyiron_atomistics\")\n",
"wf.register(\"plotting\", \"pyiron_workflow.node_library.plotting\")\n",
"wf.register(\"pyiron_workflow.node_library.pyiron_atomistics\", \"pyiron_atomistics\")\n",
"wf.register(\"pyiron_workflow.node_library.plotting\", \"plotting\")\n",
"\n",
"wf = Workflow(\"with_prebuilt\")\n",
"\n",
Expand Down
2 changes: 1 addition & 1 deletion notebooks/quickstart.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -833,7 +833,7 @@
" \"\"\"\n",
" return np.arange(n), n\n",
"\n",
"wf.register(\"plotting\", \"pyiron_workflow.node_library.plotting\")\n",
"wf.register(\"pyiron_workflow.node_library.plotting\", \"plotting\")\n",
"\n",
"wf.arange = Arange(10)\n",
"wf.plot = wf.create.plotting.Scatter(\n",
Expand Down
4 changes: 2 additions & 2 deletions pyiron_workflow/composite.py
Original file line number Diff line number Diff line change
Expand Up @@ -507,8 +507,8 @@ def _rebuild_data_io(self):

@classmethod
@wraps(Creator.register)
def register(cls, domain: str, package_identifier: str) -> None:
cls.create.register(domain=domain, package_identifier=package_identifier)
def register(cls, package_identifier: str, domain: Optional[str] = None) -> None:
cls.create.register(package_identifier=package_identifier, domain=domain)

def executor_shutdown(self, wait=True, *, cancel_futures=False):
"""
Expand Down
77 changes: 53 additions & 24 deletions pyiron_workflow/interfaces.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import pkgutil
from sys import version_info
from types import ModuleType
from typing import TYPE_CHECKING
from typing import Optional, TYPE_CHECKING

from bidict import bidict
from pyiron_workflow.snippets.singleton import Singleton
Expand Down Expand Up @@ -78,7 +78,7 @@ def __init__(self):
# in python >=3.10
# If the CI skips testing on 3.9 gets dropped, we can think about removing
# this if-clause and just letting users of python <3.10 hit an error.
self.register("standard", "pyiron_workflow.node_library.standard")
self.register("pyiron_workflow.node_library.standard", "standard")

@property
def PyFluxExecutor(self):
Expand Down Expand Up @@ -138,31 +138,53 @@ def __getattr__(self, item):
f"forget to register node package to this key?"
) from e

def __getitem__(self, item):
try:
return self._package_registry[item]
except KeyError as e:
raise KeyError(
f"Could not find the package {item} -- are you sure it's registered?"
) from e

def __getstate__(self):
return self.__dict__

def __setstate__(self, state):
self.__dict__ = state

def register(self, domain: str, package_identifier: str) -> None:
def register(self, package_identifier: str, domain: Optional[str] = None) -> None:
"""
Add a new package of nodes from the provided identifier.
The new package is available by item-access using the identifier, and, if a
domain was provided, by attribute access under that domain path ("."-split
strings allow for deep-registration of domains).
Add a new package of nodes under the provided attribute, e.g. after adding
nodes to the domain `"my_nodes"`, and instance of creator can call things like
`creator.my_nodes.some_node_that_is_there()`.

Currently, :param:`package_identifier` is just a python module string, and we
allow recursive registration of multiple node packages when a module is
provided whose sub-modules are node packages. If a :param:`domain` was
provided, then it is extended by the same semantic path as the modules, e.g.
if `my_python_module` is registered to the domain `"mpm"`, and
`my_python_module.submod1` and `my_python_module.submod2.subsub` are both node
packages, then `mpm.submod1` and `mpm.submod2.subsub` will both be available
for attribute access.

Note: If a macro is going to use a creator, the node registration should be
_inside_ the macro definition to make sure the node actually has access to
those nodes! It also needs to be _able_ to register those nodes, i.e. have
import access to that location, but we don't for that check that.

Args:
domain (str): The attribute name at which to register the new package.
(Note: no sanitizing is done here except for splitting on "." to create
sub-domains, so if you provide a string that won't work as an attribute
name, that's your problem.)
package_identifier (str): An identifier for the node package. (Right now
that's just a string version of the path to the module, e.g.
`pyiron_workflow.node_library.standard`.)
domain (str|None): The attribute name at which to register the new package.
(Note: no sanitizing is done here except for splitting on "." to create
sub-domains, so if you provide a string that won't work as an attribute
name, that's your problem.) (Default is None, don't provide attribute
access to this package.)

Raises:
KeyError: If the domain already exists, but the identifier doesn't match
Expand All @@ -179,11 +201,14 @@ def register(self, domain: str, package_identifier: str) -> None:

try:
module = import_module(package_identifier)
if "." in domain:
domain, container = self._get_deep_container(domain)
if domain is not None:
if "." in domain:
domain, container = self._get_deep_container(domain)
else:
container = self._package_access
self._register_recursively_from_module(module, domain, container)
else:
container = self._package_access
self._register_recursively_from_module(module, domain, container)
self._register_recursively_from_module(module, None, None)
except ModuleNotFoundError as e:
raise ModuleNotFoundError(
f"In the current implementation, we expect package identifiers to be "
Expand Down Expand Up @@ -212,17 +237,20 @@ def _get_deep_container(self, semantic_domain: str) -> tuple[str, DotDict]:
return domain, container

def _register_recursively_from_module(
self, module: ModuleType, domain: str, container: DotDict
self, module: ModuleType, domain: str | None, container: DotDict | None
) -> None:
if hasattr(module, "__path__"):
if domain not in container.keys():
container[domain] = DotDict()
subcontainer = container[domain]
if domain is not None:
if domain not in container.keys():
container[domain] = DotDict()
subcontainer = container[domain]
else:
subcontainer = None
for _, submodule_name, _ in pkgutil.walk_packages(
module.__path__, module.__name__ + "."
):
submodule = import_module(submodule_name)
subdomain = submodule_name.split(".")[-1]
subdomain = None if domain is None else submodule_name.split(".")[-1]

if not hasattr(submodule, "__path__"):
if hasattr(submodule, "nodes"):
Expand All @@ -239,18 +267,19 @@ def _register_recursively_from_module(
self._register_package_from_module(module, domain, container)

def _register_package_from_module(
self, module: ModuleType, domain: str, container: dict | DotDict
self, module: ModuleType, domain: str | None, container: dict | DotDict | None
) -> None:
package = self._get_existing_package_or_register_a_new_one(module.__name__)
# NOTE: Here we treat the package identifier and the module name as equivalent

if domain not in container.keys():
# If the container _doesn't_ yet have anything at this domain, just add it
container[domain] = package
else:
self._raise_error_unless_new_package_matches_existing(
container, domain, package
)
if domain is not None:
if domain not in container.keys():
# If the container _doesn't_ yet have anything at this domain, just add it
container[domain] = package
else:
self._raise_error_unless_new_package_matches_existing(
container, domain, package
)

def _get_existing_package_or_register_a_new_one(
self, package_identifier: str
Expand Down
2 changes: 1 addition & 1 deletion pyiron_workflow/node.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
NotData,
)
from pyiron_workflow.draw import Node as GraphvizNode
from pyiron_workflow.files import DirectoryObject
from pyiron_workflow.snippets.files import DirectoryObject
from pyiron_workflow.has_to_dict import HasToDict
from pyiron_workflow.io import Signals
from pyiron_workflow.topology import (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,9 @@ def __init__(self, directory):
def create(self):
self.path.mkdir(parents=True, exist_ok=True)

def delete(self):
delete_files_and_directories_recursively(self.path)
def delete(self, only_if_empty: bool = False):
if self.is_empty() or not only_if_empty:
delete_files_and_directories_recursively(self.path)

def list_content(self):
return categorize_folder_items(self.path)
Expand All @@ -71,6 +72,9 @@ def create_subdirectory(self, path):
def create_file(self, file_name):
return FileObject(file_name, self)

def is_empty(self) -> bool:
return len(self) == 0


class FileObject:
def __init__(self, file_name: str, directory: DirectoryObject):
Expand Down
2 changes: 1 addition & 1 deletion pyiron_workflow/workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ class Workflow(Composite):
namespaces. These need to be registered first, like the standard package is
automatically registered:

>>> Workflow.register("standard", "pyiron_workflow.node_library.standard")
>>> Workflow.register("pyiron_workflow.node_library.standard", "standard")

When your workflow's data follows a directed-acyclic pattern, it will determine
the execution flow automatically.
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/test_workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def sqrt(value=0):
)

def test_for_loop(self):
Workflow.register("demo", "static.demo_nodes")
Workflow.register("static.demo_nodes", "demo")

n = 5

Expand Down Expand Up @@ -188,7 +188,7 @@ def test_executor_and_creator_interaction(self):
C.f. `pyiron_workflow.function._wrapper_factory` for more detail.
"""
wf = Workflow("depickle")
wf.register("demo", "static.demo_nodes")
wf.register("static.demo_nodes", "demo")

wf.before_pickling = wf.create.demo.OptionallyAdd(1)
wf.before_pickling.executor = wf.create.Executor()
Expand Down
25 changes: 24 additions & 1 deletion tests/unit/test_files.py → tests/unit/snippets/test_files.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import unittest
from pyiron_workflow.files import DirectoryObject, FileObject
from pyiron_workflow.snippets.files import DirectoryObject, FileObject
from pathlib import Path


Expand Down Expand Up @@ -45,6 +45,29 @@ def test_is_file(self):
f.delete()
self.assertFalse(f.is_file())

def test_is_empty(self):
self.assertTrue(self.directory.is_empty())
self.directory.write(file_name="test.txt", content="something")
self.assertFalse(self.directory.is_empty())

def test_delete(self):
self.assertTrue(
Path("test").exists() and Path("test").is_dir(),
msg="Sanity check on initial state"
)
self.directory.write(file_name="test.txt", content="something")
self.directory.delete(only_if_empty=True)
self.assertFalse(
self.directory.is_empty(),
msg="Flag argument on delete should have prevented removal"
)
self.directory.delete()
self.assertFalse(
Path("test").exists(),
msg="Delete should remove the entire directory"
)
self.directory = DirectoryObject("test") # Rebuild it so the tearDown works


if __name__ == '__main__':
unittest.main()
2 changes: 1 addition & 1 deletion tests/unit/test_composite.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def bar(x: int = 0) -> int:
)

def test_creator_access_and_registration(self):
self.comp.register("demo", "static.demo_nodes")
self.comp.register("static.demo_nodes", "demo")

# Test invocation
self.comp.add_node(self.comp.create.demo.OptionallyAdd(label="by_add"))
Expand Down
Loading
Loading