Skip to content

Raise warning not error on schema version mismatch - #718

Merged
rwedge merged 8 commits into
alteryx:masterfrom
chidauri:warning
Sep 6, 2019
Merged

Raise warning not error on schema version mismatch#718
rwedge merged 8 commits into
alteryx:masterfrom
chidauri:warning

Conversation

@chidauri

@chidauri chidauri commented Aug 28, 2019

Copy link
Copy Markdown
Contributor

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

rwedge commented Sep 3, 2019

Copy link
Copy Markdown
Contributor

@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

@codecov

codecov Bot commented Sep 5, 2019

Copy link
Copy Markdown

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

chidauri commented Sep 5, 2019

Copy link
Copy Markdown
Contributor Author

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

Comment thread docs/source/changelog.rst Outdated
* 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`)

@rwedge rwedge Sep 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread featuretools/utils/gen_utils.py Outdated
break
elif c_num < s_num:
raise RuntimeError(error_text_upgrade)
warnings.warn(error_text_upgrade)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rwedge

rwedge commented Sep 5, 2019

Copy link
Copy Markdown
Contributor

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

rwedge commented Sep 5, 2019

Copy link
Copy Markdown
Contributor

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

@chidauri

chidauri commented Sep 6, 2019

Copy link
Copy Markdown
Contributor Author

@rwedge is it ok now?

@rwedge

rwedge commented Sep 6, 2019

Copy link
Copy Markdown
Contributor

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

@rwedge rwedge left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chidauri thank you for contributing!

@rwedge
rwedge merged commit ac826ea into alteryx:master Sep 6, 2019
@rwedge rwedge mentioned this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Raise warning not error on schema version mismatch

2 participants