Enable showing DeprecationWarning in debug_on and add unit test #1554

gerritholl · 2021-02-19T17:49:55Z

Extend the functionality of debug_on to switch on the visibility of DeprecationWarning using the default filter (as opposed to only showing them in __main__, which is the default Python behaviour in current and recent Python versions). Also add a unit test for debug_on.

Closes Enable showing DeprecationWarning in debug_on and add unit test #1554
Tests added
Fully documented

Add a unit test for debug_on, including the new functionality to add deprecation warnings.

Linter says docstring should be on one line. Let it be so.

ghost · 2021-02-19T19:45:16Z

Congratulations 🎉. DeepCode analyzed your code in 2.471 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

mraspaud

LGTM, thanks for taking the time. Some tests are failing though.

satpy/tests/test_utils.py

gerritholl · 2021-02-24T09:56:31Z

Hmm, I thought the tests were passing when I ran them locally :-/ I well check this.

Add a debug_off function that switches debugging off. Use this function when testing debug_on so it doesn't affect the behaviour of other unit tests. The GAC reader ones are still failing for reasons I don't understand yet.

satpy/tests/test_utils.py

satpy/utils.py

gerritholl · 2021-02-24T11:19:23Z

I am deeply confused how this PR can cause the GACLAC reader tests to fail in the way they do.

gerritholl · 2021-02-24T11:34:20Z

I have no clue why, but with this PR, these import statements all return in mocks, causing a test failure:

satpy/satpy/tests/reader_tests/test_avhrr_l1b_gaclac.py

Lines 140 to 147 in d556c80

    
           def test_init(self): 
        
               """Test GACLACFile initialization.""" 
        
               from pygac.gac_klm import GACKLMReader 
        
               from pygac.gac_pod import GACPODReader 
        
               from pygac.lac_klm import LACKLMReader 
        
               from pygac.lac_pod import LACPODReader

Any clue why that would happen with my PR, which is just doing stuff with warning filters and loggers?

codecov · 2021-02-24T11:40:48Z

Codecov Report

Merging #1554 (aaeffae) into master (d556c80) will increase coverage by 0.03%.
The diff coverage is 91.46%.

@@            Coverage Diff             @@
##           master    #1554      +/-   ##
==========================================
+ Coverage   92.54%   92.58%   +0.03%     
==========================================
  Files         251      251              
  Lines       36761    36828      +67     
==========================================
+ Hits        34022    34097      +75     
+ Misses       2739     2731       -8

Flag	Coverage Δ
behaviourtests	`4.52% <19.51%> (+0.02%)`	⬆️
unittests	`92.72% <91.46%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
satpy/resample.py	`89.50% <ø> (ø)`
satpy/scene.py	`91.65% <50.00%> (-0.61%)`	⬇️
satpy/composites/__init__.py	`88.28% <100.00%> (+0.01%)`	⬆️
satpy/tests/test_composites.py	`99.87% <100.00%> (+<0.01%)`	⬆️
satpy/tests/test_regressions.py	`100.00% <100.00%> (ø)`
satpy/tests/test_scene.py	`99.72% <100.00%> (ø)`
satpy/tests/test_utils.py	`100.00% <100.00%> (ø)`
satpy/utils.py	`88.88% <100.00%> (+6.93%)`	⬆️

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 d556c80...fbeed6e. Read the comment docs.

pnuu · 2021-02-24T11:59:13Z

The only failing build seems to be, as far as I know, the unstable build that uses bleeding edge versions from dask, xarray, etc. master branches. And that one is allowed to fail.

gerritholl · 2021-02-24T13:05:12Z

I have no clue why, but with this PR, these import statements all return in mocks, causing a test failure:

satpy/satpy/tests/reader_tests/test_avhrr_l1b_gaclac.py

Lines 140 to 147 in d556c80

def test_init(self):

"""Test GACLACFile initialization."""

from pygac.gac_klm import GACKLMReader

from pygac.gac_pod import GACPODReader

from pygac.lac_klm import LACKLMReader

from pygac.lac_pod import LACPODReader

Any clue why that would happen with my PR, which is just doing stuff with warning filters and loggers?

These failures happen on my local machine if and only if local pygac is installed, and I run the new tests:

pytest test_utils.py reader_tests/test_avhrr_l1b_gaclac.py (or a generic pytest) while pygac is installed gives me:

================================================================================================================== short test summary info ===================================================================================================================
FAILED reader_tests/test_avhrr_l1b_gaclac.py::TestGACLACFile::test__slice - ValueError: Given start line 5 exceeds scanline range 2                                                                                                                          
FAILED reader_tests/test_avhrr_l1b_gaclac.py::TestGACLACFile::test_init - AssertionError: <class 'pygac.gac_pod.GACPODReader'> is not <MagicMock name='mock.gac_pod.GACPODReader' id='140567597024208'> : Wrong reader class assigned to NSS.GHRR.NA.D79184...
FAILED reader_tests/test_avhrr_l1b_gaclac.py::TestGACLACFile::test_strip_invalid_lat - TypeError: ufunc 'isnan' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''  
========================================================================================================= 3 failed, 21 passed, 41 warnings in 6.24s ==========================================================================================================

pytest reader_test/test_avhrr_l1b_gaclac.py — all tests pass.

mraspaud · 2021-02-25T07:01:43Z

I think this deserves it's own issue. Creating now.

mraspaud

Just a few comments for clarity, otherwise it looks good.

mraspaud · 2021-02-25T07:07:18Z

satpy/utils.py

+_warning_manager = _WarningManager()
+
+
+def dep_warnings_on():


dep can be missleading (I was thinking dependency, as in warnings from other libraries), maybe just rename to deprecation_warnings_on for clarity?

mraspaud · 2021-02-25T07:07:27Z

satpy/utils.py

+    _warning_manager.filt = warnings.filters[0]
+
+
+def dep_warnings_off():


mraspaud · 2021-02-25T07:09:27Z

satpy/utils.py

+class _WarningManager:
+    """Class to handle switching warnings on and off."""
+
+    filt = None
+
+
+_warning_manager = _WarningManager()


do we really need a class here? would it work with just a module variable, eg _warning_filter?

That would have to be a global variable. I personally hate global variables. I find them confusing, error-prone, and hard to test.

In a way, isn't _warning_manager also a global variable? Yes, it's a class instance, but everything in Python is a class instance.

In a way yes, and in a way no. There is no global keyword, and I'm not changing somebody else's namespace; rather, I'm just changing the state of a mutable object that happens to be in the module namespace. The difference is subtle and in this case largely semantic, but if somebody did have the idea to use their own reference to _warning_manager (hopefully for testing purposes) it would be affected by changes, unlike a global variable which would be a name bound to a new object.

ok, I get your point.

Now, looking at the bigger picture, why use a class at all and not just a constant?
When doing warnings.filterwarnings("default", category=DeprecationWarning), the last filter becomes in principle ('default', None, DeprecationWarning, None, 0) (I say in principle because there could be a race condition, or are warning filters thread-safe?).
So we could have _deprecation_warning_filter = ('default', None, DeprecationWarning, None, 0) at the module level and use that in deprecation_warnings_off.
Or are you worried that the filter syntax will change in certain circumstances?

I don't believe it's thread safe, let's just hope people don't rely on debug_on()...debug_off() to work reliably in threaded code.

about thread-safety:

The catch_warnings manager works by replacing and then later restoring the module’s showwarning() function and internal list of filter specifications. This means the context manager is modifying global state and therefore is not thread-safe.

(last lines in https://docs.python.org/3/library/warnings.html)

Ok, I'm not thrilled with the current solution, but for lack of a better option, I say we leave it as it is.
@pnuu @djhoese any more comments on this?
Otherwise I'll merge this tonight.

No further comments from me 👍

I'm not thrilled with the Python 'warnings' module, so there is no solution that I would be thrilled with.

mraspaud · 2021-02-25T07:11:36Z

satpy/utils.py

+@contextlib.contextmanager
+def debug(dep_warnings=True):
+    """Context manager to temporarily set debugging on.
+
+    Example::
+
+        >>> with satpy.utils.debug():
+        ...     code_here()
+
+    Args:
+        dep_warnings (Optional[bool]): Switch on deprecation warnings.
+            Defaults to True.
+    """
+    debug_on()
+    yield
+    debug_off()


That's really nice, great addition!

Maybe rename dep_warnings to deprecation_warnings?

I renamed it throughout now (and passed the relevant parameter in line 93...).

gerritholl added 2 commits February 19, 2021 18:17

When debugging is on, use deprecation warnings

2ef8b92

Write unit test for debug_on

94fcfcb

Add a unit test for debug_on, including the new functionality to add deprecation warnings.

gerritholl requested review from djhoese and mraspaud as code owners February 19, 2021 17:49

Make linter happy

3eb9e19

Linter says docstring should be on one line. Let it be so.

mraspaud reviewed Feb 24, 2021

View reviewed changes

satpy/tests/test_utils.py Show resolved Hide resolved

gerritholl added 2 commits February 24, 2021 11:30

Merge branch 'master' into set-deprecation-warnings

220738a

Allow to switch debugging off

7a761e0

Add a debug_off function that switches debugging off. Use this function when testing debug_on so it doesn't affect the behaviour of other unit tests. The GAC reader ones are still failing for reasons I don't understand yet.

stickler-ci reviewed Feb 24, 2021

View reviewed changes

satpy/tests/test_utils.py Show resolved Hide resolved

satpy/utils.py Show resolved Hide resolved

satpy/utils.py Show resolved Hide resolved

gerritholl added 2 commits February 24, 2021 12:20

PEP8 fixes

85c985b

PEP 216 fixes

09d9e05

mraspaud mentioned this pull request Feb 25, 2021

Gaclac reader tests fail when pygac is installed and is run with other tests #1568

Open

mraspaud reviewed Feb 25, 2021

View reviewed changes

mraspaud assigned gerritholl Feb 25, 2021

mraspaud added the enhancement code enhancements, features, improvements label Feb 25, 2021

mraspaud added this to the v0.26.0 milestone Feb 25, 2021

Rename dep_warnings -> deprecation_warnings

fbeed6e

djhoese mentioned this pull request Feb 26, 2021

Crash when reaching warnings.DeprecationWarning #1573

Closed

mraspaud merged commit fffc4c3 into pytroll:master Mar 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable showing DeprecationWarning in debug_on and add unit test #1554

Enable showing DeprecationWarning in debug_on and add unit test #1554

gerritholl commented Feb 19, 2021

ghost commented Feb 19, 2021 •

edited by ghost

mraspaud left a comment

gerritholl commented Feb 24, 2021

gerritholl commented Feb 24, 2021

gerritholl commented Feb 24, 2021

codecov bot commented Feb 24, 2021 •

edited

pnuu commented Feb 24, 2021

gerritholl commented Feb 24, 2021

mraspaud commented Feb 25, 2021

mraspaud left a comment

mraspaud Feb 25, 2021

mraspaud Feb 25, 2021

mraspaud Feb 25, 2021

gerritholl Feb 25, 2021

pnuu Feb 25, 2021

gerritholl Feb 25, 2021

mraspaud Feb 25, 2021

gerritholl Feb 25, 2021

mraspaud Feb 25, 2021

mraspaud Feb 25, 2021 •

edited

pnuu Feb 25, 2021

gerritholl Feb 25, 2021

mraspaud Feb 25, 2021

mraspaud Feb 25, 2021

gerritholl Feb 25, 2021

		_warning_manager.filt = warnings.filters[0]


		def dep_warnings_off():

Enable showing DeprecationWarning in debug_on and add unit test #1554

Enable showing DeprecationWarning in debug_on and add unit test #1554

Conversation

gerritholl commented Feb 19, 2021

ghost commented Feb 19, 2021 • edited by ghost

👉 View analysis in DeepCode’s Dashboard | Configure the bot

mraspaud left a comment

Choose a reason for hiding this comment

gerritholl commented Feb 24, 2021

gerritholl commented Feb 24, 2021

gerritholl commented Feb 24, 2021

codecov bot commented Feb 24, 2021 • edited

Codecov Report

pnuu commented Feb 24, 2021

gerritholl commented Feb 24, 2021

mraspaud commented Feb 25, 2021

mraspaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mraspaud Feb 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 19, 2021 •

edited by ghost

codecov bot commented Feb 24, 2021 •

edited

mraspaud Feb 25, 2021 •

edited