Remove project name from schema #684

vyasr · 2022-02-21T19:34:45Z

Description

This PR updates the v1 to v2 schema migration to remove project names and store preexisting ones in the project document.

Motivation and Context

Resolves #541. Part of the changeset of #643.

Types of Changes

Documentation update
Bug fix
New feature
Breaking change¹

¹The change breaks (or has the potential to break) existing functionality.

Checklist:

I am familiar with the Contributing Guidelines.
I agree with the terms of the Contributor Agreement.
My name is on the list of contributors.
My code follows the code style guideline of this project.
The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:

I have updated the API documentation as part of the package doc-strings.
I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
I have updated the changelog and added all related issue and pull request numbers for future reference (if applicable). See example below.

vyasr · 2022-02-21T19:38:01Z

@bdice @csadorf the more I work with these schema-related PRs the more I wonder about the value in using semantic versioning-style versions for schemas. I don't think they're quite appropriate, and I think they introduce more complexity than is worthwhile, especially since there are no equivalents to "patches" AFAICT. I could theoretically imagine a schema version bump that would not require migrating from an old schema by adding support for optional keys, and that could constitute a minor version, but is that important for us to really support? I think the schema code would be significantly simplified by just using a simple integer-indexed version scheme, and that is practically what we are doing right now anyway. Thoughts?

bdice · 2022-02-21T19:58:10Z

@vyasr My recollection is that we will only make “major” (breaking) schema versions, but we use PEP 440 version identifiers (not necessarily semantic versioning) just because it gives us flexibility should we need it. I don’t have strong feelings on this. Integers would be fine with me aside from the pain of refactoring and the minor nicety of using standardized identifiers in the Python ecosystem.

vyasr · 2022-02-21T20:11:21Z

I mainly ask because the migration code ends up doing a lot of conversion back and forth between "proper" versions and integers (or really, a string containing a single number) because all of the stored variables use version numbers like "1". So I think our options are to either lean into the version numbers by replacing all of those with packaging.Version objects or to move completely away from them in favor of simple integers. @csadorf do you have a preference? I wasn't part of the early design discussions around the migration logic so I'm not sure if you two had specific thoughts at the time on the importance of full versions.

vyasr · 2022-02-21T20:56:59Z

e1e9944 addresses #643 (comment). That case isn't quite covered by the tests added in #682 since that also requires the global config file to be missing, but I think this is good enough for now. I had to change a test because in fact the behavior of various error cases handled by the config CLI wasn't consistent. The existing config code actually had some errors in it that were masked by old behaviors and were uncovered by this change, which is why we observed this problem in the first place. We should aim to improve testing of the config CLI further, but I think that's out of scope for this PR as long as we've resolved the current issues.

csadorf · 2022-02-22T09:12:25Z

@vyasr Yes, I'm in favor of just going with simple integer increments. While schemas can generally be backwards-compatible, I can't imagine a scenario where communicating compatibility through the version number is adding much value in this specific context. We can always "manually" document specific compatiblity ranges if there is a need for this.

vyasr · 2022-02-23T17:10:36Z

OK, I'll make a separate PR with that change.

signac/__main__.py

signac/contrib/migration/__init__.py

signac/contrib/project.py

bdice · 2022-02-25T16:55:37Z

tests/test_project.py

@@ -98,7 +98,7 @@ def test_repr(self):
        assert p == self.project

    def test_str(self):
-        str(self.project) == self.project.id
+        str(self.project) == f"Project.get_project({self.project.root_directory()})"


This needs to use a repr so that the path is quoted, based on the implementation. I haven't checked if tests are passing.

Suggested change

str(self.project) == f"Project.get_project({self.project.root_directory()})"

str(self.project) == f"Project.get_project({repr(self.project.root_directory())})"

Well it would help if this test actually contained an assertion... 😂

Looks like this test has remained unchanged since bbf7e7b way back in 2016 (when it was first added). Probably good to enable it now... 🙃

To know oneself, one should assert oneself.
-- Albert Camus

codecov · 2022-02-26T05:17:03Z

Codecov Report

Merging #684 (c8cc307) into schema2 (cf73d1c) will decrease coverage by 0.04%.
The diff coverage is 74.07%.

❗ Current head c8cc307 differs from pull request most recent head ca697f8. Consider uploading reports for the commit ca697f8 to get more accurate results

@@             Coverage Diff             @@
##           schema2     #684      +/-   ##
===========================================
- Coverage    86.28%   86.24%   -0.05%     
===========================================
  Files           52       52              
  Lines         5040     5037       -3     
  Branches      1101     1099       -2     
===========================================
- Hits          4349     4344       -5     
- Misses         489      491       +2     
  Partials       202      202

Impacted Files	Coverage Δ
signac/common/validate.py	`100.00% <ø> (ø)`
signac/__main__.py	`78.16% <56.25%> (+0.48%)`	⬆️
signac/contrib/migration/v1_to_v2.py	`92.00% <100.00%> (-8.00%)`	⬇️
signac/contrib/project.py	`89.29% <100.00%> (-0.16%)`	⬇️
signac/contrib/migration/__init__.py	`82.75% <0.00%> (-3.45%)`	⬇️
signac/common/config.py	`78.12% <0.00%> (-3.13%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf73d1c...ca697f8. Read the comment docs.

bdice

A few more thoughts. I'm fine if you merge this once you've considered/applied my comments, so I'm approving.

bdice · 2022-02-26T05:08:13Z

signac/__main__.py

-            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


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)

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.

bdice · 2022-02-26T05:14:11Z

signac/__main__.py

-    verify_config(cfg)
+            mode = "local or global"
+        _print_err(f"Did not find a {mode} configuration file.")
+        return


Suggested change

return

sys.exit(1)

signac/contrib/project.py

* Remove project id API. * Remove project name from config as part of migration. * Fix issues with config CLI and remove project from default cfg. * Address PR comments. * Change the str of a project to the str of its root directory.

* Initial migration to schema version 2 including config rename (#678) * Implement initial migration to schema version 2. * Require project-local config to be signac.rc (not .signacrc) and make searches stricter to match. * Standardize method for getting project config at a root. * Move config to .signac/config. * Fix import order. * Address PR comments. * Remove some unnecessary code. * Address final PR coments. * Use integer schema version numbers (#688) * Change schema versioning to use integer strings. * Switch from int strings to pure ints. * Update signac/contrib/migration/__init__.py Co-authored-by: Bradley Dice <bdice@bradleydice.com> Co-authored-by: Bradley Dice <bdice@bradleydice.com> * Remove project name from schema (#684) * Remove project id API. * Remove project name from config as part of migration. * Fix issues with config CLI and remove project from default cfg. * Address PR comments. * Change the str of a project to the str of its root directory. * Change Project constructor to use root directory (#706) * Change project constructor to accept a root directory instead of a config file. * Change Project repr. * Address easy PR comments. * Move internal signac files into .signac subdirectory (#708) * Move shell history. * Move sp_cache file. * Address PR comments. * Move discovery to separate functions. (#711) * Move discovery to separate functions. * Address first round of PR comments. * Address PR comments. * Apply suggestions. * Remove configurable workspace directory (#714) * Remove workspace configurability. * Implement workspace_dir migration. * Apply suggestions from code review Co-authored-by: Bradley Dice <bdice@bradleydice.com> * Address remaining PR comments. * Update tests/test_project.py * Remove mention of configurability from project workspace docstring * Address PR comments. Co-authored-by: Bradley Dice <bdice@bradleydice.com> Co-authored-by: Corwin Kerr <cbkerr@umich.edu> * Update description of schema migration. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: Bradley Dice <bdice@bradleydice.com> Co-authored-by: Corwin Kerr <cbkerr@umich.edu> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Remove project id API. * Remove project name from config as part of migration. * Fix issues with config CLI and remove project from default cfg. * Address PR comments. * Change the str of a project to the str of its root directory.

vyasr added this to the v2.0.0 milestone Feb 21, 2022

vyasr requested review from a team as code owners February 21, 2022 19:34

vyasr self-assigned this Feb 21, 2022

vyasr requested review from bdice and pepak13 and removed request for a team February 21, 2022 19:34

vyasr force-pushed the schema2 branch from de94095 to 47dc740 Compare February 21, 2022 19:41

vyasr requested a review from a team as a code owner February 21, 2022 19:41

vyasr force-pushed the schema/remove_project_name branch from 7ad3e2e to 6b02258 Compare February 21, 2022 19:41

vyasr force-pushed the schema2 branch from 47dc740 to 01b5b53 Compare February 21, 2022 20:19

vyasr force-pushed the schema/remove_project_name branch from 6b02258 to 578d433 Compare February 21, 2022 20:19

csadorf removed the request for review from a team February 22, 2022 09:05

vyasr mentioned this pull request Feb 24, 2022

Use integer schema version numbers #688

Merged

12 tasks

bdice reviewed Feb 25, 2022

View reviewed changes

vyasr added 4 commits February 25, 2022 20:39

Remove project id API.

257205a

Remove project name from config as part of migration.

982e8c0

Fix issues with config CLI and remove project from default cfg.

f02df5d

Address PR comments.

c8cc307

vyasr force-pushed the schema/remove_project_name branch from e1e9944 to c8cc307 Compare February 26, 2022 04:58

vyasr requested a review from bdice February 26, 2022 04:58

bdice approved these changes Feb 26, 2022

View reviewed changes

Change the str of a project to the str of its root directory.

ca697f8

vyasr merged commit 01873a1 into schema2 Feb 26, 2022

vyasr deleted the schema/remove_project_name branch February 26, 2022 05:42

vyasr mentioned this pull request Mar 4, 2022

Removing signac project id #541

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove project name from schema #684

Remove project name from schema #684

vyasr commented Feb 21, 2022

vyasr commented Feb 21, 2022

bdice commented Feb 21, 2022 •

edited

Loading

vyasr commented Feb 21, 2022

vyasr commented Feb 21, 2022

csadorf commented Feb 22, 2022

vyasr commented Feb 23, 2022

bdice Feb 25, 2022

vyasr Feb 26, 2022 •

edited

Loading

bdice Feb 26, 2022

codecov bot commented Feb 26, 2022 •

edited

Loading

bdice left a comment

bdice Feb 26, 2022

vyasr Feb 26, 2022 •

edited

Loading

bdice Feb 26, 2022

	str(self.project) == f"Project.get_project({self.project.root_directory()})"
	str(self.project) == f"Project.get_project({repr(self.project.root_directory())})"

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

Remove project name from schema #684

Remove project name from schema #684

Conversation

vyasr commented Feb 21, 2022

Description

Motivation and Context

Types of Changes

Checklist:

vyasr commented Feb 21, 2022

bdice commented Feb 21, 2022 • edited Loading

vyasr commented Feb 21, 2022

vyasr commented Feb 21, 2022

csadorf commented Feb 22, 2022

vyasr commented Feb 23, 2022

bdice Feb 25, 2022

Choose a reason for hiding this comment

vyasr Feb 26, 2022 • edited Loading

Choose a reason for hiding this comment

bdice Feb 26, 2022

Choose a reason for hiding this comment

codecov bot commented Feb 26, 2022 • edited Loading

Codecov Report

bdice left a comment

Choose a reason for hiding this comment

bdice Feb 26, 2022

Choose a reason for hiding this comment

vyasr Feb 26, 2022 • edited Loading

Choose a reason for hiding this comment

bdice Feb 26, 2022

Choose a reason for hiding this comment

bdice commented Feb 21, 2022 •

edited

Loading

vyasr Feb 26, 2022 •

edited

Loading

codecov bot commented Feb 26, 2022 •

edited

Loading

vyasr Feb 26, 2022 •

edited

Loading