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

Remove project name from schema #684

Merged
merged 5 commits into from
Feb 26, 2022
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
41 changes: 18 additions & 23 deletions signac/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,7 @@
SHELL_BANNER = """Python {python_version}
signac {signac_version} 🎨

Project:\t{project_id}{job_banner}
Root:\t\t{root_path}
Project:\t{root_path}{job_banner}
Workspace:\t{workspace_path}
Size:\t\t{size}

Expand Down Expand Up @@ -549,7 +548,6 @@ def _main_import_interactive(project, origin, args):
banner=SHELL_BANNER_INTERACTIVE_IMPORT.format(
python_version=sys.version,
signac_version=__version__,
project_id=project.id,
job_banner="",
root_path=project.root_directory(),
workspace_path=project.workspace(),
Expand Down Expand Up @@ -724,16 +722,14 @@ def main_config_show(args):
cfg = config.read_config_file(config.USER_CONFIG_FN)
else:
cfg = config.load_config()
if cfg is None:
if args.local and args.globalcfg:
mode = " local or global "
elif args.local:
mode = " local "
if not cfg:
if args.local:
mode = "local"
elif args.globalcfg:
mode = " global "
mode = "global"
else:
mode = ""
_print_err(f"Did not find a{mode}configuration file.")
mode = "local or global"
_print_err(f"Did not find a {mode} configuration file.")
return
Copy link
Member

Choose a reason for hiding this comment

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

Should this exit with some nonzero return code? I notice that some of our code paths calling _print_err are informational, while others indicate actual errors and are followed by sys.exit(1). Some code paths raise RuntimeWarning, which also exits with code 1.

signac/signac/__main__.py

Lines 1715 to 1719 in c8cc307

except RuntimeWarning as warning:
_print_err(f"Warning: {warning}")
if args.debug:
raise
sys.exit(1)

Perhaps we need to assess this in a separate PR, and make sure all the code in __main__.py is consistent about using nonzero exit codes for errors. For now I'll mark a couple cases that are obvious.

Suggested change
return
sys.exit(1)

Copy link
Contributor Author

@vyasr vyasr Feb 26, 2022

Choose a reason for hiding this comment

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

I read the first paragraph and was going to say "yes, I realized this too but decided that was better suited to a separate PR", and then I read your second paragraph. But... I'll do it anyway: yes, I realized this too but decided that was better suited to a separate PR!

It definitely is something that we should fix, though.

for key in args.key:
for kt in key.split("."):
Expand All @@ -759,19 +755,19 @@ def main_config_verify(args):
cfg = config.read_config_file(config.USER_CONFIG_FN)
else:
cfg = config.load_config()
if cfg is None:
if args.local and args.globalcfg:
mode = " local or global "
elif args.local:
mode = " local "
if not cfg:
if args.local:
mode = "local"
elif args.globalcfg:
mode = " global "
mode = "global"
else:
mode = ""
raise RuntimeWarning(f"Did not find a{mode}configuration file.")
if cfg.filename is not None:
_print_err(f"Verification of config file '{cfg.filename}'.")
verify_config(cfg)
mode = "local or global"
_print_err(f"Did not find a {mode} configuration file.")
return
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
return
sys.exit(1)

else:
if cfg.filename is not None:
_print_err(f"Verification of config file '{cfg.filename}'.")
verify_config(cfg)


def main_config_set(args):
Expand Down Expand Up @@ -897,7 +893,6 @@ def write_history_file():
banner=SHELL_BANNER.format(
python_version=sys.version,
signac_version=__version__,
project_id=project.id,
job_banner=f"\nJob:\t\t{job.id}" if job is not None else "",
root_path=project.root_directory(),
workspace_path=project.workspace(),
Expand Down
1 change: 0 additions & 1 deletion signac/common/validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ def get_validator(): # noqa: D103


cfg = """
project = string()
workspace_dir = string(default='workspace')
schema_version = string(default='1')
"""
14 changes: 13 additions & 1 deletion signac/contrib/migration/v1_to_v2.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,14 @@

from signac.common import configobj
from signac.common.config import _get_project_config_fn
from signac.contrib.project import Project
from signac.synced_collections.backends.collection_json import BufferedJSONAttrDict

from .v0_to_v1 import _load_config_v1

# A minimal v2 config.
_cfg = """
schema_version = string(default='0')
project = string()
workspace_dir = string(default='workspace')
"""

Expand All @@ -34,6 +37,15 @@ def _load_config_v2(root_directory):

def _migrate_v1_to_v2(root_directory):
"""Migrate from schema version 1 to version 2."""
# Delete project name from config and store in project doc.
cfg = _load_config_v1(root_directory)
fn_doc = os.path.join(root_directory, Project.FN_DOCUMENT)
doc = BufferedJSONAttrDict(filename=fn_doc, write_concern=True)
doc["signac_project_name"] = cfg["project"]
del cfg["project"]
cfg.write()

# Move signac.rc to .signac/config
v1_fn = os.path.join(root_directory, "signac.rc")
v2_fn = _get_project_config_fn(root_directory)
os.mkdir(os.path.dirname(v2_fn))
Expand Down
37 changes: 5 additions & 32 deletions signac/contrib/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,17 +121,6 @@ def __init__(self, config=None):
self._config = _ProjectConfig(config)
self._lock = RLock()

# Ensure that the project id is configured.
try:
self._id = str(self.config["project"])
except KeyError:
raise LookupError(
"Unable to determine project id. "
"Please verify that '{}' is a signac project path.".format(
os.path.abspath(self.config.get("project_dir", os.getcwd()))
)
)

# Ensure that the project's data schema is supported.
self._check_schema_compatibility()

Expand All @@ -156,8 +145,8 @@ def __init__(self, config=None):
_mkdir_p(self.workspace())
except OSError:
logger.error(
"Error occurred while trying to create "
"workspace directory for project {}.".format(self.id)
"Error occurred while trying to create workspace directory for project at root "
f"{self.root_directory()}."
)
raise

Expand All @@ -175,8 +164,7 @@ def __init__(self, config=None):
)

def __str__(self):
"""Return the project's id."""
return str(self.id)
return str(self.root_directory())

def __repr__(self):
return "{type}.get_project({root})".format(
Expand All @@ -194,8 +182,7 @@ def _repr_html_(self):
"""
return (
"<p>"
+ f"<strong>Project:</strong> {self.id}<br>"
+ f"<strong>Root:</strong> {self.root_directory()}<br>"
+ f"<strong>Project:</strong> {self.root_directory()}<br>"
+ f"<strong>Workspace:</strong> {self.workspace()}<br>"
+ f"<strong>Size:</strong> {len(self)}"
+ "</p>"
Expand Down Expand Up @@ -255,18 +242,6 @@ def workspace(self):
"""
return self._workspace

@property
def id(self):
"""Get the project identifier.

Returns
-------
str
The project id.

"""
return self._id

def _check_schema_compatibility(self):
"""Check whether this project's data schema is compatible with this version.

Expand Down Expand Up @@ -1573,15 +1548,13 @@ def init_project(cls, *args, root=None, workspace=None, make_dir=True, **kwargs)
if make_dir:
_mkdir_p(os.path.dirname(fn_config))
config = read_config_file(fn_config)
config["project"] = name
if workspace is not None:
config["workspace_dir"] = workspace
config["schema_version"] = SCHEMA_VERSION
config.write()
project = cls.get_project(root=root)
if name is not None:
project.doc[name_key] = name
assert project.id == str(name)
return project
else:
if workspace is not None and os.path.realpath(
Expand Down Expand Up @@ -1625,7 +1598,7 @@ def get_project(cls, root=None, search=True, **kwargs):
if root is None:
root = os.getcwd()
config = load_config(root=root, local=False)
if "project" not in config or (
if "project_dir" not in config or (
not search
and os.path.realpath(config["project_dir"]) != os.path.realpath(root)
):
Expand Down
14 changes: 1 addition & 13 deletions tests/test_project.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,17 +88,13 @@ class TestProjectBase(TestJobBase):


class TestProject(TestProjectBase):
def test_get(self):
pass

def test_repr(self):
repr(self.project)
p = eval(repr(self.project))
assert repr(p) == repr(self.project)
assert p == self.project

def test_str(self):
str(self.project) == self.project.id
assert str(self.project) == self.project.root_directory()

def test_root_directory(self):
assert self._tmp_pr == self.project.root_directory()
Expand Down Expand Up @@ -2290,14 +2286,6 @@ def test_get_project(self):
assert project.workspace() == os.path.join(root, "workspace")
assert project.root_directory() == root

def test_project_no_id(self):
root = self._tmp_dir.name
signac.init_project(root=root)
config = load_config(root)
del config["project"]
with pytest.raises(LookupError):
Project(config=config)

def test_get_project_non_local(self):
root = self._tmp_dir.name
subdir = os.path.join(root, "subdir")
Expand Down
9 changes: 2 additions & 7 deletions tests/test_shell.py
Original file line number Diff line number Diff line change
Expand Up @@ -792,14 +792,9 @@ def test_config_set(self):

def test_config_verify(self):
# no config file
with pytest.raises(ExitCodeError):
self.call("python -m signac config --local verify".split(), error=True)
err = self.call(
"python -m signac config --local verify".split(),
error=True,
raise_error=False,
)
err = self.call("python -m signac config --local verify".split(), error=True)
assert "Did not find a local configuration file" in err

self.call("python -m signac init my_project".split())
err = self.call("python -m signac config --local verify".split(), error=True)
assert "Passed" in err
Expand Down