replace logger.warning with warnings.warn #1040

systemshift · 2020-06-30T05:25:36Z

Pull Request Description

Based on #1014 and the linked stack overflow thread discussion.
I started with the intent where warnings.warn() should be used for designing a library(which featuretools is) to warn the user when it is not used correctly. Where as logger.warning() is more suitable for apps where the wrong input is user data.

So most of warnings.warn() seem to be used as they should be, with a few cases where logger was used and I felt warnings.warn() made more sense.
Now there are a few cases of logger.warning() that looked off, but I did not want to change them, so I would like someone to review the remaining logger.warning() cases to see if it should still be there and not replaced.

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.

codecov · 2020-07-05T09:10:08Z

Codecov Report

Merging #1040 into main will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1040   +/-   ##
=======================================
  Coverage   98.35%   98.36%           
=======================================
  Files         126      126           
  Lines       13098    13123   +25     
=======================================
+ Hits        12883    12908   +25     
  Misses        215      215

Impacted Files	Coverage Δ
featuretools/feature_base/feature_base.py	`97.94% <ø> (ø)`
...computational_backends/calculate_feature_matrix.py	`99.07% <100.00%> (ø)`
featuretools/entityset/entity.py	`96.19% <100.00%> (ø)`
featuretools/entityset/entityset.py	`97.65% <100.00%> (+<0.01%)`	⬆️
featuretools/primitives/options_utils.py	`100.00% <100.00%> (ø)`
...utational_backend/test_calculate_feature_matrix.py	`98.21% <100.00%> (+0.01%)`	⬆️
featuretools/tests/entityset_tests/test_es.py	`99.85% <100.00%> (+<0.01%)`	⬆️
...ests/primitive_tests/test_features_deserializer.py	`100.00% <100.00%> (ø)`
...ols/tests/synthesis/test_deep_feature_synthesis.py	`99.21% <100.00%> (-0.01%)`	⬇️
featuretools/utils/gen_utils.py	`100.00% <100.00%> (ø)`

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 2e04b54...4e68ae4. Read the comment docs.

systemshift · 2020-07-05T09:15:53Z

I will consider this PR done and ready to merge unless someone points out to warnings worth changing.

thehomebrewnerd · 2020-07-07T16:31:11Z

@systemshift Please pull in the latest changes from the main branch, and you may need to update changelog.rst after this to get your update in the right location.

thehomebrewnerd

I am approaching this with the idea that if the user should do something differently we will use warnings.warn, but if there is nothing the user can/should do then we will use logger.warn.

Here are the warnings I think should be changed:

featuretools/utils/gen_utils.py L73 - change this to logger.warn as there is nothing the user can do if the saved schema is no longer supported
featuretools/computational_backends/calculate_feature_matrix L560 - change to logger.warn as this isn't caused by the user
featuretools/entityset/entity.py L549 - use warnings.warn as the user can fix this by setting the last time index
featuretools/entityset/entity.py L578 - use warnings.warn as the user can fix this by specifying the index column
featuretools/entityset/entity.py L588 - use warnings.warn as the user can fix this by specifying the parameters correctly
featuretools/feature_basefeature_base.py` L174 - remove commented out warning that is not used
featuretools/primitives/options_utils.py L63 - use warnings.warn as this can be fixed if the user does not specify redundant primitive options

systemshift · 2020-07-16T16:41:11Z

featuretools/primitives/options_utils.py

-                  "primitive class (%s), primitive instance will not use generic " \
+                  "primitive class %s, primitive instance will not use generic " \


This one was really hard to work around, using () in a string seems to break something with pytest.warn. Tried using string literals but no luck.

Would like to know if someone knows how to work around this issue.

I don't think it is critical to have the parentheses here, but I'm also not sure how to work around this exactly either.

using escapes on the parentheses -- "\(%s\)" -- in the warning_msg string should work. The string is interpreted as a regex so a regular parentheses won't get the match we want

thehomebrewnerd · 2020-07-16T19:46:56Z

featuretools/tests/entityset_tests/test_es.py

-    if warning_text:
-        with pytest.warns(UserWarning) as record:
-            deserialize.description_to_entityset(dictionary)
-        assert record[0].message.args[0] == warning_text
+    # TODO Refactor this function and both parent functions to run tests with a less complex implementation
+    earlier_schema_warning = 'is no longer supported by this version'
+    if earlier_schema_warning in str(warning_text):
+        logger = logging.getLogger('featuretools')
+        logger.propagate = True
+        deserialize.description_to_entityset(dictionary)
+        assert warning_text in caplog.text
+        logger.propagate = False


Before we were checking that a UserWarning got raised for both earlier and later schema versions with the assert statement inside the pytest.warns context manager. Now we changed one of these to be logged instead.

With these code changes here, we are only testing that warning is logged for the earlier schema test. We are no longer testing that the UserWarning still gets raised for the later schema test. To test that we need to add a later scheme condition to the if-else block to verify the warning gets raised as it did before.

One way you could refactor this would be to split _check_schema_version into three different functions. You could do something like _check_schema_raises_warning to check that the UserWarning gets raised inside test_later_schema_version. Then have _check_schema_logs_warning to test logging for the test_earlier_schema_version test. In both of those new utility functions we could use a third _get_schema_dictionary function to build and return the dictionary that will be needed by both of the new check functions.

Another idea would be to create a new parameter that gets passed to _check_schema_version to indicate whether we should check for a UserWarning or a logged warning instead of relying on the value of the warning text to make that determination.

This seems like it is getting more complicated than I anticipated though. @rwedge Do you have any thoughts on the best approach here?

I think keep _check_schema_version as a single function, add a new parameter to it to indicate if it is expected to warn / log / neither, and use warning_text for matching purposes

thehomebrewnerd · 2020-07-16T19:52:27Z

featuretools/tests/primitive_tests/test_features_deserializer.py

-    if warning_text:
-        with pytest.warns(UserWarning) as record:
-            FeaturesDeserializer(dictionary)
-
-        assert record[0].message.args[0] == warning_text
+    # TODO Refactor this function and both parent functions to run tests with a less complex implementation
+    earlier_schema_warning = 'is no longer supported by this version'
+    if earlier_schema_warning in str(warning_text):
+        logger = logging.getLogger('featuretools')
+        logger.propagate = True
+        FeaturesDeserializer(dictionary)
+        assert warning_text in caplog.text
+        logger.propagate = False


Similar to the entityset test, we need to make sure we are testing for the UserWarning for the later schema version test.

thehomebrewnerd · 2020-07-16T19:53:40Z

featuretools/primitives/options_utils.py

-                  "primitive class (%s), primitive instance will not use generic " \
+                  "primitive class %s, primitive instance will not use generic " \


I don't think it is critical to have the parentheses here, but I'm also not sure how to work around this exactly either.

rwedge

Thanks for making these changes, @systemshift! I identified a few cases where warnings weren't be tested before, it would be great to add explicit warning checking to these tests.

featuretools/entityset/entityset.py

featuretools/entityset/entity.py

featuretools/tests/entityset_tests/test_es.py

rwedge

Looks good! Thanks for the contribution!

systemshift added 3 commits June 29, 2020 22:16

replace logger.warning with warnings.warn

f3c3e4a

update changelog

2feb1c9

fix changelog typo

5be2dd9

rwedge changed the base branch from master to main July 2, 2020 18:29

systemshift added 2 commits July 5, 2020 00:47

revert to using logging.warning in __init__

6dc0e2e

edit warning message to work with warnings.warn()

be33339

systemshift added 2 commits July 7, 2020 10:22

Merge branch 'main' into warning_and_logger

a864fea

update changelog

a7ef17c

thehomebrewnerd requested changes Jul 7, 2020

View reviewed changes

systemshift added 12 commits July 9, 2020 00:17

replace warnings with logging in gen_utils

3f4079c

use correct logger

2099359

use caplog to capture warning(still not working)

a8b3031

lint

aab8eff

capture logs and pass tests

00b5600

pass logging tests

614cd31

replace scatter_warning with logging and pass tests

1a8f4b8

replace logging with warn in _handle_time

201f822

replace logging with warn in _create_index

e6bbbf0

replace logger with warn in _create_index

87fc805

remove logged comment from feature_base.py

b5ef2be

replace logger with warn in options_utils

dd2e120

systemshift commented Jul 16, 2020

View reviewed changes

systemshift added 2 commits July 16, 2020 09:42

Merge branch 'main' into warning_and_logger

8cb4d9a

remove str()

a95a4bd

systemshift requested a review from thehomebrewnerd July 16, 2020 16:58

thehomebrewnerd requested changes Jul 16, 2020

View reviewed changes

add parentheses without breaking regex in string

22e7d22

systemshift added 2 commits July 18, 2020 16:34

check if log or warn cases

415f486

Merge branch 'main' into warning_and_logger

843a869

systemshift requested a review from rwedge July 19, 2020 00:01

rwedge reviewed Jul 20, 2020

View reviewed changes

featuretools/entityset/entityset.py Show resolved Hide resolved

featuretools/entityset/entity.py Show resolved Hide resolved

featuretools/entityset/entity.py Show resolved Hide resolved

featuretools/entityset/entity.py Show resolved Hide resolved

systemshift added 5 commits July 21, 2020 18:15

Merge branch 'main' into warning_and_logger

fd69259

update test_cannot_re_add_relationships with warn

55ea8c2

update test_training_window with warn

545b3c3

update test_none_index with warn

00c5517

update test_unknown_index with warn

91056ac

systemshift requested a review from rwedge July 22, 2020 03:09

Merge branch 'main' into warning_and_logger

2287575

rwedge suggested changes Jul 22, 2020

View reviewed changes

featuretools/tests/entityset_tests/test_es.py Outdated Show resolved Hide resolved

systemshift added 3 commits July 22, 2020 17:31

move pytest.warn to the line above warning

21b6cdc

move pytest.warn above warning line

6dc5f53

move pytest.warnabove line

4e68ae4

systemshift requested a review from rwedge July 23, 2020 00:48

rwedge approved these changes Jul 23, 2020

View reviewed changes

rwedge merged commit be08f88 into alteryx:main Jul 23, 2020

systemshift deleted the warning_and_logger branch July 23, 2020 03:07

thehomebrewnerd mentioned this pull request Jul 24, 2020

Verify proper usage of warnings.warn() and logger.warning() throughout code #1014

Closed

rwedge mentioned this pull request Jul 31, 2020

v0.18.0 #1098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace logger.warning with warnings.warn #1040

replace logger.warning with warnings.warn #1040

systemshift commented Jun 30, 2020

codecov bot commented Jul 5, 2020 •

edited

systemshift commented Jul 5, 2020

thehomebrewnerd commented Jul 7, 2020

thehomebrewnerd left a comment •

edited

systemshift Jul 16, 2020

thehomebrewnerd Jul 16, 2020

rwedge Jul 17, 2020 •

edited

thehomebrewnerd Jul 16, 2020

thehomebrewnerd Jul 16, 2020 •

edited

rwedge Jul 17, 2020

thehomebrewnerd Jul 16, 2020

thehomebrewnerd Jul 16, 2020

rwedge left a comment

rwedge left a comment

		"primitive class (%s), primitive instance will not use generic " \
		"primitive class %s, primitive instance will not use generic " \

replace logger.warning with warnings.warn #1040

replace logger.warning with warnings.warn #1040

Conversation

systemshift commented Jun 30, 2020

Pull Request Description

codecov bot commented Jul 5, 2020 • edited

Codecov Report

systemshift commented Jul 5, 2020

thehomebrewnerd commented Jul 7, 2020

thehomebrewnerd left a comment • edited

Choose a reason for hiding this comment

systemshift Jul 16, 2020

Choose a reason for hiding this comment

thehomebrewnerd Jul 16, 2020

Choose a reason for hiding this comment

rwedge Jul 17, 2020 • edited

Choose a reason for hiding this comment

thehomebrewnerd Jul 16, 2020

Choose a reason for hiding this comment

thehomebrewnerd Jul 16, 2020 • edited

Choose a reason for hiding this comment

rwedge Jul 17, 2020

Choose a reason for hiding this comment

thehomebrewnerd Jul 16, 2020

Choose a reason for hiding this comment

thehomebrewnerd Jul 16, 2020

Choose a reason for hiding this comment

rwedge left a comment

Choose a reason for hiding this comment

rwedge left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 5, 2020 •

edited

thehomebrewnerd left a comment •

edited

rwedge Jul 17, 2020 •

edited

thehomebrewnerd Jul 16, 2020 •

edited