Migrate using project directories instead of project instances

[vyasr]

Description

This PR changes the schema migration code to operate in terms of a project's root directory rather than a project class instance. Additionally, loading and checking the schema version of a project is done using a cascade of multiple schema loaders that support different schemas.

Motivation and Context

The current schema migration logic makes some easily invalidated assumptions about what operations are valid in the course of a migration. The two most problematic assumptions are that 1) config.load_config can load schemas of any schema version, and 2) a valid Project can be created from any schema version by simply skipping the schema compatibility check. The former is invalidated by, for instance, a change in the format of the config file like the one planned for schema version 2. The latter would be invalidated by any change to the Project constructor that assumes the existence of certain keys in the config. The migration to schema version 2 does not immediately break this logic since we are removing project ids, not adding them (see #643 and #644), but there is no reason to assume that we will not make changes in the opposite direction in future schema versions. This PR protects us against such changes.

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]

@csadorf @jennyfothergill apologies in advance if the quality isn’t up to my usual standard. It’s been hard to dedicate a significant block of time, so I’ve been working on this in very brief fits and starts and I figured at this point it would be better to just get feedback on what I had rather than wait longer.

[codecov]

Codecov Report

Merging #654 (096daba) into master (44f59ee) will increase coverage by 0.00%.
The diff coverage is 81.03%.

@@           Coverage Diff           @@
##           master     #654   +/-   ##
=======================================
  Coverage   78.42%   78.42%           
=======================================
  Files          65       65           
  Lines        7119     7129   +10     
  Branches     1564     1564           
=======================================
+ Hits         5583     5591    +8     
- Misses       1226     1230    +4     
+ Partials      310      308    -2     

Impacted Files	Coverage Δ
signac/__main__.py	71.89% <ø> (ø)
signac/contrib/migration/__init__.py	83.05% <80.00%> (+0.59%)	⬆️
signac/contrib/migration/v0_to_v1.py	83.33% <81.81%> (-16.67%)	⬇️
signac/contrib/project.py	85.20% <100.00%> (-0.02%)	⬇️

---

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 44f59ee...096daba. Read the comment docs.

[bdice]

I have a handful of comments. I think this looks pretty good overall. It's unfortunate that the logic for migrations is so complex, but I can't envision a simpler design.

[csadorf]

Codecov is complaining a lot about missing tests. Maybe we can restructure the existing tests to more systematically cover those additional cases?

[vyasr]

Codecov is complaining a lot about missing tests. Maybe we can restructure the existing tests to more systematically cover those additional cases?

Thanks for pointing this out. I've gone through the untested cases and improved test coverage where appropriate. The remaining untested cases fall into two buckets:

1. Various lines in _get_config_schema_version that will start getting triggered as soon as we have more than two versions and more than a single migration. I'm comfortable leaving these untested for now with the expectation that additional tests in Remove project id #643 will start covering these.
2. Two exception case in apply_migrations itself. The FileNotFoundError on the lock file I think is something we don't have to do anything about. We frequently leave those types of cases untested in signac since they're there to case subtle filesystem errors rather than logic errors. The other exception case is there to handle the scenario when a migration fails. This code is also IMO not worth testing until Remove project id #643. If either of you can think of a reasonable scenario where we wouldn't fail in _collect_migrations (i.e. an invalid schema version) but where the v1->v2 migration could fail, I'm happy to add that to Remove project id #643.

In general the lack of coverage just reflects that we've added migration logic before we had a need for it in anticipation of future use cases. I made these changes on master because they would have cluttered the PR actually introducing the v1->v2 migration and made them hard to review, and the lack of testability is the tradeoff. We should see that improve with #643 (and I'm sure we'll find additional bugs then, but at least the core logic in place now seems sound).

There is one other change of note that I introduced. While attempting to improve coverage, I realized that because apply_migrations was defined as a generator it was a no-op unless the result was iterated over, i.e. apply_migrations(root) does nothing unless it's wrapped with something like list(apply_migrations(root)) or for (origin, destination) in apply_migrations(root): .... That API doesn't make any sense to me, and being able to iterate over the intermediate states of migration is IMO only really useful for testing purposes, so I've removed the yield statement in apply_migrations so that it simply performs all migrations at once. Let me know if either of you foresee any issues with that or would prefer a different resolution.

[bdice]

I'm happy with this PR. As noted before, it's a little hard to review these changes because we don't actually have a migration defined yet. The code looks stable and general enough, and there is a significant benefit to having this merged to help unblock other work. Let's find the remaining sharp edges when we introduce a real migration!

[vyasr]

@csadorf let me know if you'd like to re-review this. It would be fine IMO to merge this, then work out any further kinks in the context of #643 since that will give us our first real migration to stress test this with anyway, but if you see other issues that you think are worth addressing in the context of this PR we can deal with them first.

[csadorf]

@csadorf let me know if you'd like to re-review this. It would be fine IMO to merge this, then work out any further kinks in the context of #643 since that will give us our first real migration to stress test this with anyway, but if you see other issues that you think are worth addressing in the context of this PR we can deal with them first.

Thanks, I will review it right now.

[csadorf]

Looks very good overall, just a few minor questions.

[vyasr]

@csadorf I'm treating our discussion at the meeting as approval and I'm going to go ahead and merge this PR now.