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

Raise warning not error on schema version mismatch #718

Merged
merged 8 commits into from Sep 6, 2019

Conversation

@chidauri
Copy link
Contributor

commented Aug 28, 2019

Pull Request Description

Fixes #698 .


After creating the pull request: in order to pass the changelog_updated check you will need to update the "Future Release" section of docs/source/changelog.rst to include this pull request.

@rwedge

This comment has been minimized.

Copy link
Collaborator

commented Sep 3, 2019

@chidauri would you be willing to modify the tests to check that the warning is raised? Here's an example of how we check for a warning in a different test:

https://github.com/Featuretools/featuretools/blob/7d9da1533a6cb7f81dc9feb521c44834c033c530/featuretools/tests/computational_backend/test_calculate_feature_matrix.py#L43-L47

@chidauri chidauri force-pushed the chidauri:warning branch from 4d770cf to 4af4f75 Sep 5, 2019
chidauri added 2 commits Sep 5, 2019
@codecov

This comment has been minimized.

Copy link

commented Sep 5, 2019

Codecov Report

Merging #718 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #718      +/-   ##
==========================================
+ Coverage   97.64%   97.64%   +<.01%     
==========================================
  Files         118      118              
  Lines       10247    10249       +2     
==========================================
+ Hits        10006    10008       +2     
  Misses        241      241
Impacted Files Coverage Δ
featuretools/utils/gen_utils.py 93.54% <100%> (+0.14%) ⬆️
...ests/primitive_tests/test_features_deserializer.py 100% <100%> (ø) ⬆️
featuretools/tests/entityset_tests/test_es.py 100% <100%> (ø) ⬆️

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 1de047b...ff3102d. Read the comment docs.

@chidauri

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@rwedge python2.7 test is failing, i am not sure how is that related to my change? and how to format changelog.rst? Thanks.

@@ -9,12 +9,13 @@ Changelog
* Fixed entity set deserialization (:pr:`720`)
* Added error message when DateTimeIndex is a variable but not set as the time_index (:pr:`723`)
* Changes
* Raise warning and not error on schema version mismatch (:pr: `718`)

This comment has been minimized.

Copy link
@rwedge

rwedge Sep 5, 2019

Collaborator

Remove the space between ":pr:" and "718"

@@ -112,15 +113,15 @@ def check_schema_version(cls, cls_type):
if c_num > s_num:
break
elif c_num < s_num:
raise RuntimeError(error_text_upgrade)
warnings.warn(error_text_upgrade)

This comment has been minimized.

Copy link
@rwedge

rwedge Sep 5, 2019

Collaborator

We should add a break after this warning so that we do not continue to iterate after the warning

@rwedge

This comment has been minimized.

Copy link
Collaborator

commented Sep 5, 2019

Also, let's rephrase these warnings. Let's remove the "unable to load" sentence from both warnings and add "Attempting to load . . ." at the end of each warning.

@rwedge

This comment has been minimized.

Copy link
Collaborator

commented Sep 5, 2019

@chidauri the python2.7 failure was unrelated. I re-triggered the tests and it passed the second time.

@chidauri

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

@rwedge is it ok now?

@rwedge

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2019

@chidauri looks good! I went ahead and changed the error_text variable names to warning_text for some added clarity.

@rwedge
rwedge approved these changes Sep 6, 2019
Copy link
Collaborator

left a comment

@chidauri thank you for contributing!

@rwedge rwedge merged commit ac826ea into FeatureLabs:master Sep 6, 2019
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 97.64%)
Details
codecov/project 97.64% (+<.01%) compared to 1de047b
Details
license/cla Contributor License Agreement is signed.
Details
test_all_python_versions Workflow: test_all_python_versions
Details
@rwedge rwedge referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.