Add schema version #253

bdice · 2019-12-06T21:19:19Z

Description

Adds an explicit schema version for the signac data model.

Motivation and Context

Resolves #165.

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.

Example for a changelog entry: Fix issue with launching rockets to the moon (#101, #212).

bdice · 2019-12-06T21:21:24Z

@csadorf I used the Version class and parse_version function, but I'm not sure if I like it. It adds "final" to the end of the version numbers when printed, like 1.0.0final.

codecov · 2019-12-06T21:27:02Z

Codecov Report

Merging #253 into master will increase coverage by 0.31%.
The diff coverage is 90.12%.

@@            Coverage Diff            @@
##           master    #253      +/-   ##
=========================================
+ Coverage   65.08%   65.4%   +0.31%     
=========================================
  Files          39      41       +2     
  Lines        5588    5654      +66     
=========================================
+ Hits         3637    3698      +61     
- Misses       1951    1956       +5

Impacted Files	Coverage Δ
signac/__main__.py	`0% <ø> (ø)`	⬆️
signac/common/validate.py	`39.13% <ø> (-4.21%)`	⬇️
signac/contrib/errors.py	`94.73% <100%> (+0.61%)`	⬆️
signac/contrib/migration/v0_to_v1.py	`100% <100%> (ø)`
signac/version.py	`100% <100%> (ø)`	⬆️
signac/contrib/migration/__init__.py	`87.27% <87.27%> (ø)`
signac/contrib/project.py	`90.88% <94.73%> (+0.01%)`	⬆️

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 cea2eff...15ce09d. Read the comment docs.

csadorf · 2019-12-06T21:28:53Z

@csadorf I used the Version class and parse_version function, but I'm not sure if I like it. It adds "final" to the end of the version numbers when printed, like 1.0.0final.

Under what circumstances does this happen? For the warnings and error messages?

csadorf · 2019-12-06T21:34:58Z

I just did a quick search, I'm not sure where we used that class anyways, maybe use distutils.version.StrictVersion instead?

bdice · 2019-12-06T21:35:58Z

@csadorf I used the Version class and parse_version function, but I'm not sure if I like it. It adds "final" to the end of the version numbers when printed, like 1.0.0final.

Under what circumstances does this happen? For the warnings and error messages?

Right, in the errors/warnings. To see it, just initialize a new project and then edit the config file to a higher major/minor version.

bdice · 2019-12-06T21:38:49Z

@csadorf Yeah, distutils is another choice. I am unavailable for most of the weekend, so if you’re interested in trying it, go ahead and commit to this branch. We may want to check if that Version class is used elsewhere and deprecate it if distutils does the job.

csadorf · 2019-12-07T00:37:58Z

I'll have a look at it.

csadorf · 2019-12-07T12:16:55Z

I realized that there are few things we didn't fully consider. I'm almost done with implementing the missing pieces. Will push soon.

bdice · 2019-12-09T16:14:29Z

@csadorf Reminder to push your changes when convenient.

csadorf

I was the last one to push changes, so I am erroneously still requested for review...

bdice · 2019-12-11T02:48:47Z

signac/__main__.py

@@ -19,6 +19,8 @@
 import errno
 from pprint import pprint, pformat

+from packaging import version


https://stackoverflow.com/a/11887885
According to this StackOverflow post, packaging isn't in the standard library but comes as a dependency of setuptools. I just wanted to make a note of that.

Yes, so we should probably add setuptools to the dependencies, however, it is not clear to me if we need to require a specific version.

@csadorf I looked at the history of the setuptools git repo and changelog. packaging has been included as a vendored package (not a dependency) since setuptools v8.0 (Dec 13, 2014). I think that StackOverflow answer I linked above is out of date. I recommend that we add packaging as a dependency instead. It's a very common dependency so I expect it will be already met by most users' environments.

But then in v39 it was changed to dependencies again?

Just tested, packaging is not installed by default in a "fresh" conda environment, but it's certainly very easy to install.

I tested this and we need packaging>=15.0.

bdice

@csadorf Since we're hoping to include this in the release on Friday, I am offering some preliminary feedback - I know tests aren't passing yet but I hope this is still helpful.

signac/__main__.py

signac/contrib/errors.py

signac/__main__.py

signac/contrib/migrations.py

signac/contrib/project.py

csadorf · 2019-12-11T16:16:11Z

@bdice I plan on finalizing this before Friday. I need a free minute to consider how to potentially reduce the footprint of this patch (your concern).

I'll ping you again when I think it's ready and will ask you to informally review it again. I realize that this PR is kind of turned around now, sorry about that.

bdice · 2019-12-11T16:20:08Z

I realize that this PR is kind of turned around now, sorry about that.

Definitely not a problem - my goal was only to provide a draft implementation for further consideration. Thanks for taking it on.

csadorf · 2019-12-12T14:24:44Z

@bdice This should be ready now. Please have a look.

bdice

@csadorf Two change requests (I can't request changes on my own PR). I want to use proper semantic versions, e.g. '1.0.0' instead of '1'. Also we need to make the requirements.txt file match the requirements in setup.py.

bdice · 2019-12-13T03:21:32Z

signac/common/validate.py

@@ -52,7 +41,7 @@ def get_validator():
 cfg = """
 project = string()
 workspace_dir = string(default='workspace')
-signac_version = version(default='0,1,0')
+schema_version = string(default='1')


Should this be a proper semantic version string?

Suggested change

schema_version = string(default='1')

schema_version = string(default='1.0.0')

See comment below.

csadorf · 2019-12-13T08:27:25Z

@csadorf Two change requests (I can't request changes on my own PR). I want to use proper semantic versions, e.g. '1.0.0' instead of '1'.

I opted to use a simpler system, because I don't think that semantic versioning actually makes sense in this context. What would it mean to have a minor or patch release of the scheme? Is there ever a scenario where you could operate on a minor or patch release without migrating?

I compare this to other API schemes, that usually use a simple sequential scheme. Many RESTful APIs use a single number to indicate which API version they are using. I don't think that semantic versioning makes sense in this context.

Refactoring.

To reduce footprint.

So that code related to a particular migration can be kept within one module.

With correct minimal version (tested).

And add unit test to check that the code is enabled when necessary.

Using the project instance provided by the unit test class.

To get more control over and information about the migration process.

bdice · 2019-12-13T17:16:07Z

@csadorf Two change requests (I can't request changes on my own PR). I want to use proper semantic versions, e.g. '1.0.0' instead of '1'.

I opted to use a simpler system, because I don't think that semantic versioning actually makes sense in this context. What would it mean to have a minor or patch release of the scheme? Is there ever a scenario where you could operate on a minor or patch release without migrating?

I compare this to other API schemes, that usually use a simple sequential scheme. Many RESTful APIs use a single number to indicate which API version they are using. I don't think that semantic versioning makes sense in this context.

I agree that using an integer version (not a string version) is preferable, but then we don't need packaging at all, and there are other places where semantic versions are used but aren't needed.

csadorf · 2019-12-13T17:25:44Z

@csadorf Two change requests (I can't request changes on my own PR). I want to use proper semantic versions, e.g. '1.0.0' instead of '1'.

I opted to use a simpler system, because I don't think that semantic versioning actually makes sense in this context. What would it mean to have a minor or patch release of the scheme? Is there ever a scenario where you could operate on a minor or patch release without migrating?
I compare this to other API schemes, that usually use a simple sequential scheme. Many RESTful APIs use a single number to indicate which API version they are using. I don't think that semantic versioning makes sense in this context.

I agree that using an integer version (not a string version) is preferable, but then we don't need packaging at all, and there are other places where semantic versions are used but aren't needed.

Agreed, I'll make that change.

csadorf · 2019-12-13T17:35:31Z

@csadorf Two change requests (I can't request changes on my own PR). I want to use proper semantic versions, e.g. '1.0.0' instead of '1'.

I opted to use a simpler system, because I don't think that semantic versioning actually makes sense in this context. What would it mean to have a minor or patch release of the scheme? Is there ever a scenario where you could operate on a minor or patch release without migrating?
I compare this to other API schemes, that usually use a simple sequential scheme. Many RESTful APIs use a single number to indicate which API version they are using. I don't think that semantic versioning makes sense in this context.

I agree that using an integer version (not a string version) is preferable, but then we don't need packaging at all, and there are other places where semantic versions are used but aren't needed.

Agreed, I'll make that change.

We determined that packaging was already inadvertently introduced as dependency as part of PR #253 so we'll leave this as is.

bdice

@csadorf Requesting changes: use integer versions everywhere, instead of mixing '1' and '1.0.0' styles.

signac/version.py

tests/test_project.py

bdice requested a review from a team as a code owner December 6, 2019 21:19

bdice requested review from csadorf and removed request for a team December 6, 2019 21:19

bdice self-assigned this Dec 6, 2019

bdice added the enhancement New feature or request label Dec 6, 2019

bdice added this to the v1.3.0 milestone Dec 6, 2019

csadorf reviewed Dec 10, 2019

View reviewed changes

bdice commented Dec 11, 2019

View reviewed changes

csadorf requested a review from a team as a code owner December 11, 2019 15:43

csadorf requested review from yuanzhou0827 and removed request for a team December 11, 2019 15:43

csadorf force-pushed the feature/schema-version branch from 3422698 to 8a7b7db Compare December 11, 2019 16:13

csadorf force-pushed the feature/schema-version branch 2 times, most recently from fd79706 to f2f2426 Compare December 12, 2019 13:53

bdice commented Dec 13, 2019

View reviewed changes

bdice assigned csadorf Dec 13, 2019

csadorf added 14 commits December 13, 2019 09:57

Remove redundant doc-string comment.

12eb2f4

Check for correct exception type in unit test.

62d0927

Rename the contrib.migrations module to contrib.migration.

d9408e8

Move locking for migration into migration module.

5f43426

Refactoring.

Add unit test for migration.

a0e3a39

Remove 'contrib.migration.MIGRATION' from public API.

39e2ea9

Replace migration class with migration function.

e8e1329

To reduce footprint.

Convert 'migration' module to package.

8d6644b

So that code related to a particular migration can be kept within one module.

Add 'packaging' to requirements.

cda3136

With correct minimal version (tested).

Minor refactorization of the migrate/__init__.py module.

de09bd9

Comment out migration related code.

3ac966d

And add unit test to check that the code is enabled when necessary.

Streamline migration unit tests.

43a3392

Using the project instance provided by the unit test class.

Make the apply_migrations() function a generator.

a15f078

To get more control over and information about the migration process.

Add 'filelock' and 'packaging' to requirements.txt.

c84c7cf

csadorf force-pushed the feature/schema-version branch from a901ea4 to c84c7cf Compare December 13, 2019 08:57

csadorf approved these changes Dec 13, 2019

View reviewed changes

vyasr mentioned this pull request Dec 13, 2019

Implement execution hooks framework glotzerlab/signac-flow#189

Closed

12 tasks

bdice commented Dec 13, 2019

View reviewed changes

signac/version.py Outdated Show resolved Hide resolved

tests/test_project.py Outdated Show resolved Hide resolved

Use integer strings instead of full semantic versions for consistency.

c04d471

bdice requested a review from a team as a code owner December 13, 2019 21:15

bdice added 2 commits December 13, 2019 16:16

Merge branch 'master' into feature/schema-version

9a56741

Merge branch 'master' into feature/schema-version

15ce09d

bdice merged commit 6ef0a98 into master Dec 14, 2019

bdice deleted the feature/schema-version branch December 14, 2019 01:05

bdice mentioned this pull request Feb 27, 2021

Refactor/reorganize root dir #517

Closed

12 tasks

vyasr mentioned this pull request Mar 9, 2021

Update signac's internal schema #527

Closed

4 tasks

vyasr mentioned this pull request Apr 25, 2021

Add schema versioning to flow glotzerlab/signac-flow#504

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add schema version #253

Add schema version #253

bdice commented Dec 6, 2019 •

edited

Loading

bdice commented Dec 6, 2019

codecov bot commented Dec 6, 2019 •

edited

Loading

csadorf commented Dec 6, 2019

csadorf commented Dec 6, 2019

bdice commented Dec 6, 2019

bdice commented Dec 6, 2019

csadorf commented Dec 7, 2019

csadorf commented Dec 7, 2019

bdice commented Dec 9, 2019

csadorf left a comment

bdice Dec 11, 2019

csadorf Dec 11, 2019

bdice Dec 11, 2019

csadorf Dec 11, 2019

csadorf Dec 12, 2019

bdice left a comment •

edited

Loading

csadorf commented Dec 11, 2019

bdice commented Dec 11, 2019

csadorf commented Dec 12, 2019

bdice left a comment

bdice Dec 13, 2019

csadorf Dec 13, 2019

csadorf commented Dec 13, 2019

bdice commented Dec 13, 2019

csadorf commented Dec 13, 2019

csadorf commented Dec 13, 2019

bdice left a comment •

edited

Loading

	schema_version = string(default='1')
	schema_version = string(default='1.0.0')

Add schema version #253

Add schema version #253

Conversation

bdice commented Dec 6, 2019 • edited Loading

Description

Motivation and Context

Types of Changes

Checklist:

bdice commented Dec 6, 2019

codecov bot commented Dec 6, 2019 • edited Loading

Codecov Report

csadorf commented Dec 6, 2019

csadorf commented Dec 6, 2019

bdice commented Dec 6, 2019

bdice commented Dec 6, 2019

csadorf commented Dec 7, 2019

csadorf commented Dec 7, 2019

bdice commented Dec 9, 2019

csadorf left a comment

Choose a reason for hiding this comment

bdice Dec 11, 2019

Choose a reason for hiding this comment

csadorf Dec 11, 2019

Choose a reason for hiding this comment

bdice Dec 11, 2019

Choose a reason for hiding this comment

csadorf Dec 11, 2019

Choose a reason for hiding this comment

csadorf Dec 12, 2019

Choose a reason for hiding this comment

bdice left a comment • edited Loading

Choose a reason for hiding this comment

csadorf commented Dec 11, 2019

bdice commented Dec 11, 2019

csadorf commented Dec 12, 2019

bdice left a comment

Choose a reason for hiding this comment

bdice Dec 13, 2019

Choose a reason for hiding this comment

csadorf Dec 13, 2019

Choose a reason for hiding this comment

csadorf commented Dec 13, 2019

bdice commented Dec 13, 2019

csadorf commented Dec 13, 2019

csadorf commented Dec 13, 2019

bdice left a comment • edited Loading

Choose a reason for hiding this comment

bdice commented Dec 6, 2019 •

edited

Loading

codecov bot commented Dec 6, 2019 •

edited

Loading

bdice left a comment •

edited

Loading

bdice left a comment •

edited

Loading