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.

Sign up for GitHub

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

Jump to bottom

Change Project constructor to use root directory #706

Merged
merged 3 commits into from
Mar 5, 2022
Merged

Change Project constructor to use root directory #706

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
72015c1
Change project constructor to accept a root directory instead of a co…
vyasr Dec 19, 2021
7559513
Change Project repr.
vyasr Dec 26, 2021
94759e9
Address easy PR comments.
vyasr Mar 5, 2022
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
32 changes: 17 additions & 15 deletions signac/contrib/project.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,16 +89,17 @@ def __setitem__(self, key, value):
class Project:
"""The handle on a signac project.

Application developers should usually not need to
directly instantiate this class, but use
:meth:`~signac.get_project` instead.
A :class:`Project` may only be constructed in a directory that is already a
signac project, i.e. a directory in which :func:`~signac.init_project` has
already been run. If project discovery (searching upwards in the folder
hierarchy until a project root is discovered) is desired, users should
instead invoke :func:`~signac.get_project` or :meth:`Project.get_project`.

Parameters
----------
config :
The project configuration to use. By default, it loads the first signac
project configuration found while searching upward from the current
working directory (Default value = None).
root : str, optional
The project root directory. By default, the current working directory
vyasr marked this conversation as resolved.
Show resolved Hide resolved
(Default value = None).

"""

Expand All @@ -115,9 +116,12 @@ class Project:

_use_pandas_for_html_repr = True # toggle use of pandas for html repr

def __init__(self, config=None):
if config is None:
config = load_config()
def __init__(self, root=None):
if root is None:
root = os.getcwd()
# Project constructor does not search upward, so the provided root must
# be a project directory.
config = load_config(root, local=True)
self._config = _ProjectConfig(config)
self._lock = RLock()

Expand Down Expand Up @@ -167,9 +171,7 @@ def __str__(self):
return str(self.root_directory())

def __repr__(self):
return "{type}.get_project({root})".format(
type=self.__class__.__name__, root=repr(self.root_directory())
)
return f"{self.__class__.__name__}({repr(self.root_directory())})"

def _repr_html_(self):
"""Project details in HTML format for use in IPython environment.
Expand Down Expand Up @@ -1597,7 +1599,7 @@ def get_project(cls, root=None, search=True, **kwargs):
"""
if root is None:
root = os.getcwd()
config = load_config(root=root, local=False)
config = load_config(root=root, local=not search)
if "project_dir" not in config or (
not search
and os.path.realpath(config["project_dir"]) != os.path.realpath(root)
Expand All @@ -1608,7 +1610,7 @@ def get_project(cls, root=None, search=True, **kwargs):
)
)

return cls(config=config, **kwargs)
return cls(root=config["project_dir"], **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

The current approach finds and loads a project config and then uses its "project_dir" key to construct the Project. Can this be simplified somehow, so that we check for config existence and then pass the root to the constructor? We shouldn't need to load the config twice (in get_project and again in the constructor), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm that's a good point. This question is related to the last piece of #643, which is changing load_config to not populate the project_dir into the config but to instead change load_config to return a tuple of (root_dir, config). The reason I originally wanted to make that change is because actually modifying the contents of the config with something that isn't contained in any of the config files is an unexpected side effect that could cause confusion. Perhaps your question here is an indication that the discovery aspect should be separated into a completely separate function, and that load_config would stop discovering altogether? Alternatively, load_config could still be allowed to discover even if we added a separate discover_config function, but that seems like unnecessary redundancy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bdice let me know what you think here. I can split those changes into a separate PR if that's the direction that we want to go.

Copy link
Member

@bdice bdice Mar 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it might be a better design to separate discovery from config loading (parsing). A separate PR might be best, I agree.


@classmethod
def get_job(cls, root=None):
Expand Down