Detect Schema for different type of Project to Sync #280

vishav1771 · 2020-01-29T12:29:46Z

Description

Changed the detect_schema function to compare on the bases of data types instead of values.

Motivation and Context

Fixes #230

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).

vishav1771 · 2020-01-29T12:34:58Z

Hi @csadorf @vyasr, I have added tests to show the new behavior of sync function with different types of projects. Is this behavior fine or need to change anything?

vyasr · 2020-01-29T18:38:45Z

@vishav1771 it looks like your tests are failing because the docstring you added to your test has an extra trailing quote (four instead of three). I'll look at the tests now.

tests/test_sync.py

vishav1771 · 2020-01-29T18:54:42Z

@vyasr I have not done any changes in the detect_schema yet. I just wanted to confirm the intended behavior of sync of function (as shown by the test ) is correct or not?

vyasr · 2020-01-29T19:01:58Z

Yes, you've absolutely done the right thing. To make sure I understand these tests:

test_mixed_same_data_type will fail with current signac, but we believe it should pass.
test_mixed_diff_data_type will fail with current signac and we believe it should fail
test_mixed_multiple_data_type will fail with current signac, and you think it should succeed in one direction but not the other (the direction where one project's key-values are a strict superset of the other).

Is that interpretation correct? One more I might add is where the types don't match but the values match exactly e.g. float(1) and int(1) as the values. It would also be good to test a case where one has a superset of the other's keys, i.e. both projects have a={1, 2}, but one project also has jobs with b=1.

Reading these tests makes me think that we are perhaps too stringent with our tests of invalid schema. The existence of the check_schema flag perhaps makes that acceptable, however it's not clear to me what a user of this would expect in all of these cases. For instance, I might naively only expect synchronization to fail if two jobs with the same statepoint fail to synchronize, i.e. I'm wondering if maybe the expected default for most users is simply check_schema=False. I would like to get some feedback from more users/developers on how they would expect synchronization to work. @csadorf would you be OK with polling a few others before making a decision on the "correct" behavior?

csadorf · 2020-01-30T08:51:06Z

Reading these tests makes me think that we are perhaps too stringent with our tests of invalid schema.

I agree. The initial intention when I added the schema check was to prevent cases where one would apply a migration to one project data space (project A) and then sync it with a diverged copy (project B), e.g., one where a key was renamed, and now one would end up with a bunch of additional jobs that were originally supposed to be the same.

So here is my proposal, I think that we should keep the current behavior where sync really only syncs when the schema is exactly the same. However, we should clarify in the error message that this does not meany anything bad if you intend to sync two data space that are similar, but not identical in terms of schema. We could consider defusing the warning message and to use a different option to override just that check that sounds less menacing then -f/--force, something like -m/--merge or so.

When I look at the original issue, the error message that we see is very confusing:

Keys found only in the source schema: dt, totalSteps, k_bending, period, phi, seed, a, reversalRate, totalMonomers, f_p
Keys found only in the destination schema: dt, totalSteps, k_bending, period, phi, seed, a, reversalRate, totalMonomers, f_p

That just doesn't make any sense since it claims that certain keys were only found in the source or destination schema, but they are actually identical.

This is what needs to be addressed.

vyasr · 2020-01-30T14:23:03Z

I think that's a reasonable approach for now. If we go this route of not changing how schema checking works, long term I would advocate that for 2.0 we change the default behavior to check_schema=False. I think that is the behavior people are actually expecting. As long as the per-job synchronization is robust with respect to conflicts, the worst thing people can do is end up with duplicate jobs that can be easily removed or migrated correctly using the signac shell interface.

vishav1771 · 2020-01-30T19:29:10Z

@vyasr So, Only need to change the warning message and a different option to override for now?

vyasr · 2020-01-30T20:21:50Z

Yes, I think that for now we would just want to add a --no-check-schema option and then change the text of the error.

csadorf

Please update this PR based on master and apply discussed modifications to this PR.

signac/__main__.py

codecov · 2020-02-03T10:46:50Z

Codecov Report

Merging #280 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #280      +/-   ##
==========================================
+ Coverage   64.81%   64.86%   +0.05%     
==========================================
  Files          40       40              
  Lines        5644     5644              
==========================================
+ Hits         3658     3661       +3     
+ Misses       1986     1983       -3

Impacted Files	Coverage Δ
signac/contrib/project.py	`90.54% <0%> (+0.42%)`	⬆️

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 c845639...074723f. Read the comment docs.

signac/__main__.py

csadorf

Great! Approved pending the application of the minor suggestions that I added.

@vishav1771 One final question, have you verified that this will solve the specific issue for the data space posted in #230 ?

signac/__main__.py

vishav1771 · 2020-02-04T12:42:49Z

@vishav1771 One final question, have you verified that this will solve the specific issue for the data space posted in #230 ?

Yes, I have verified it with statepoints given in #230 and few other cases. However, while testing I came across a test case shown :

project1:
{
 job1:{
    'a' : 0,
    'b' : 0,
  }
 job2:{
    'a' : 1,
    'b' : 1,
  }
}

project2:
{
 job1:{
    'a' : 0,
    'b' : 1,
  }
 job2:{
    'a' : 1,
    'b' : 0,
  }
}

They will generate the same schema and will pass check_schema. I don't think that this is the intended behaviour. Can you please verify?

csadorf · 2020-02-04T13:53:58Z

@vishav1771 One final question, have you verified that this will solve the specific issue for the data space posted in #230 ?

Yes, I have verified it with statepoints given in #230 and few other cases.

Great, thank you.

They will generate the same schema and will pass check_schema. I don't think that this is the intended behaviour. Can you please verify?

While the two projects don't have the identical set of jobs, their implicit schema is indeed the same so IMO it is intended behavior that they would sync without problem. Otherwise we would need to redefine that merging means that one syncs two projects that have the exact same set of jobs. I don't think that's what we want. @vyasr Thoughts?

vyasr · 2020-02-04T15:28:33Z

@vishav1771 One final question, have you verified that this will solve the specific issue for the data space posted in #230 ?
They will generate the same schema and will pass check_schema. I don't think that this is the intended behaviour. Can you please verify?

While the two projects don't have the identical set of jobs, their implicit schema is indeed the same so IMO it is intended behavior that they would sync without problem. Otherwise we would need to redefine that merging means that one syncs two projects that have the exact same set of jobs. I don't think that's what we want. @vyasr Thoughts?

I agree. I think this question is revisiting the motivation for the original requirement that schema had to be exactly identical, which was to avoid e.g. renaming or adding keys to schema and then trying to sync. If the keys and the types of the values corresponding to each key are the same between two schema, any jobs that exist in the source but not the destination must be truly distinct statepoints, so I think this behavior is the correct one.

csadorf · 2020-02-04T16:21:43Z

@vyasr Great, please merge at your discretion.

vyasr

I found a typo, I'm going to apply the suggestion myself then I think it's good to go.

signac/__main__.py

vyasr · 2020-02-04T17:27:36Z

@vishav1771 I don't have push permissions to your repo. Can you commit my suggestion directly? I will merge after that.

Also, in general it is helpful for us if you complete the checklist when you make the pull request. That lets us track how far you've gotten.

vishav1771 · 2020-02-04T17:35:54Z

@vishav1771 I don't have push permissions to your repo. Can you commit my suggestion directly? I will merge after that.

Sure

Also, in general it is helpful for us if you complete the checklist when you make the pull request. That lets us track how far you've gotten.

Sorry about that. I'll keep in mind for next time

csadorf · 2020-02-10T14:37:19Z

@vyasr This is ready, isn't it?

vyasr · 2020-02-10T14:56:41Z

Yes, thanks for the ping.

vyasr

LGTM, thanks!

test for sych schema

78e76a0

vishav1771 requested a review from csadorf January 29, 2020 12:36

bdice assigned vishav1771 Jan 29, 2020

vyasr reviewed Jan 29, 2020

View reviewed changes

tests/test_sync.py Outdated Show resolved Hide resolved

Changed get_statepoint

01f0987

Changes

748e2e1

csadorf requested changes Jan 31, 2020

View reviewed changes

vishav1771 added 6 commits February 1, 2020 00:57

Updating Branch

afb507f

Changed error msg for SchemaSyncConflict

27fa827

Updating

0cef357

Updated

721912d

Removed tests

4398d85

Linting Error

cf9b1f0

vishav1771 marked this pull request as ready for review January 31, 2020 21:51

vishav1771 requested review from a team as code owners January 31, 2020 21:51

vishav1771 requested review from Tobias-Dwyer, csadorf and vyasr and removed request for a team January 31, 2020 21:51

csadorf requested changes Feb 3, 2020

View reviewed changes

signac/__main__.py Outdated Show resolved Hide resolved

added force

9835fde

vishav1771 added 2 commits February 3, 2020 16:22

Error for -f

fa1eea0

Linting Error

7b812a0

vishav1771 requested a review from csadorf February 3, 2020 11:08

csadorf requested changes Feb 3, 2020

View reviewed changes

signac/__main__.py Outdated Show resolved Hide resolved

Changed no_check_schema to merge

416f02f

vishav1771 requested review from csadorf and removed request for Tobias-Dwyer February 3, 2020 19:00

csadorf approved these changes Feb 4, 2020

View reviewed changes

signac/__main__.py Outdated Show resolved Hide resolved

signac/__main__.py Outdated Show resolved Hide resolved

Error msg corrected

c1bc8c3

vyasr approved these changes Feb 4, 2020

View reviewed changes

signac/__main__.py Outdated Show resolved Hide resolved

vyasr self-requested a review February 4, 2020 17:26

typo

c845639

vyasr approved these changes Feb 10, 2020

View reviewed changes

Merge branch 'master' into sync_schema_check

074723f

vyasr merged commit 2e73c78 into glotzerlab:master Feb 10, 2020

bdice added this to the v1.4.0 milestone Feb 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect Schema for different type of Project to Sync #280

Detect Schema for different type of Project to Sync #280

vishav1771 commented Jan 29, 2020 •

edited

Loading

vishav1771 commented Jan 29, 2020 •

edited

Loading

vyasr commented Jan 29, 2020

vishav1771 commented Jan 29, 2020

vyasr commented Jan 29, 2020 •

edited

Loading

csadorf commented Jan 30, 2020

vyasr commented Jan 30, 2020

vishav1771 commented Jan 30, 2020

vyasr commented Jan 30, 2020 •

edited

Loading

csadorf left a comment

codecov bot commented Feb 3, 2020 •

edited

Loading

csadorf left a comment

vishav1771 commented Feb 4, 2020

csadorf commented Feb 4, 2020

vyasr commented Feb 4, 2020

csadorf commented Feb 4, 2020

vyasr left a comment

vyasr commented Feb 4, 2020

vishav1771 commented Feb 4, 2020

csadorf commented Feb 10, 2020

vyasr commented Feb 10, 2020

vyasr left a comment

Detect Schema for different type of Project to Sync #280

Detect Schema for different type of Project to Sync #280

Conversation

vishav1771 commented Jan 29, 2020 • edited Loading

Description

Motivation and Context

Types of Changes

Checklist:

vishav1771 commented Jan 29, 2020 • edited Loading

vyasr commented Jan 29, 2020

vishav1771 commented Jan 29, 2020

vyasr commented Jan 29, 2020 • edited Loading

csadorf commented Jan 30, 2020

vyasr commented Jan 30, 2020

vishav1771 commented Jan 30, 2020

vyasr commented Jan 30, 2020 • edited Loading

csadorf left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 3, 2020 • edited Loading

Codecov Report

csadorf left a comment

Choose a reason for hiding this comment

vishav1771 commented Feb 4, 2020

csadorf commented Feb 4, 2020

vyasr commented Feb 4, 2020

csadorf commented Feb 4, 2020

vyasr left a comment

Choose a reason for hiding this comment

vyasr commented Feb 4, 2020

vishav1771 commented Feb 4, 2020

csadorf commented Feb 10, 2020

vyasr commented Feb 10, 2020

vyasr left a comment

Choose a reason for hiding this comment

vishav1771 commented Jan 29, 2020 •

edited

Loading

vishav1771 commented Jan 29, 2020 •

edited

Loading

vyasr commented Jan 29, 2020 •

edited

Loading

vyasr commented Jan 30, 2020 •

edited

Loading

codecov bot commented Feb 3, 2020 •

edited

Loading