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

Use warnings module for deprecation warning #444

Merged
merged 3 commits into from
Oct 30, 2020

Conversation

coroa
Copy link
Collaborator

@coroa coroa commented Oct 20, 2020

Please confirm that this PR has done the following:

  • Tests Added -> single line change, deprecation_warning function did not have tests before
  • Documentation Added
  • Description in RELEASE_NOTES.md Added ?

Description of PR

The new deprecation warnings are quite intense (i could attach a screenshot). The warnings module has a few benefits over them. Notably:

  • Has the stacklevel attribute to highlight the place where the user has to adapt her code
  • Only warns once per line of code, where it is misused
  • One can easily filter out the warnings or even raise them as an error
import warnings
warnings.filterwarnings(message="This method is deprecated.*", action="error")

I did not (yet) include all the tick-boxes. Can do, if you think it makes sense?

pyam/logging.py Outdated Show resolved Hide resolved
@coroa coroa force-pushed the use-warning-module branch from ac14d5a to da2b669 Compare October 20, 2020 23:25
@danielhuppmann
Copy link
Member

Thanks @coroa, I agree with the sentiment of making deprecation-warnings less annoying - but when I run this in a Jupyter notebook, I don't get any warnings at all, which defeats the purpose.

Am I doing something wrong, or can you modify the settings such that it shows by default in Jupyter notebooks, like we do for the logger...

get_ipython().run_cell_magic(u'javascript', u'',

@gidden
Copy link
Member

gidden commented Oct 21, 2020

Hi both, and thanks @coroa !

I just tried installing the branch and in a fresh notebook I executed the following:

from pyam.logging import deprecation_warning
deprecation_warning('foo')

and the following came out:

pyam - INFO: Running in a notebook, setting `pyam` logging level to `logging.INFO` and adding stderr handler
/Users/matthewgidden/miniconda3/lib/python3.7/site-packages/ipykernel_launcher.py:2: DeprecationWarning: This method is deprecated and will be removed in future versions. foo

This is on a mac, so is there any chance it could be an OS issue?

@gidden
Copy link
Member

gidden commented Oct 21, 2020

Happy to test here if there are other snippets

@danielhuppmann
Copy link
Member

import pandas as pd
import pyam

TEST_DF = pd.DataFrame([
    ['model_a', 'scen_a', 'World', 'Primary Energy', 'EJ/yr', 1, 6.],
    ['model_a', 'scen_a', 'World', 'Primary Energy|Coal', 'EJ/yr', 0.5, 3],
    ['model_a', 'scen_b', 'World', 'Primary Energy', 'EJ/yr', 2, 7],
],
    columns=pyam.IAMC_IDX + [2005, 2010],
)

df = pyam.IamDataFrame(TEST_DF)
df.models()

This does not produce a warning on my Mac OS.

@danielhuppmann
Copy link
Member

Sorry, I pushed to this branch by mistake - hope that I cleaned it up correctly...

@danielhuppmann danielhuppmann added the logging Issues related to logging label Oct 26, 2020
@coroa
Copy link
Collaborator Author

coroa commented Oct 26, 2020

Thanks @coroa, I agree with the sentiment of making deprecation-warnings less annoying - but when I run this in a Jupyter notebook, I don't get any warnings at all, which defeats the purpose.

Am I doing something wrong, or can you modify the settings such that it shows by default in Jupyter notebooks, like we do for the logger...

get_ipython().run_cell_magic(u'javascript', u'',

Sorry for the slow response time, I was on holidays for a couple of days. I'll come to the logger in a separate PR. There were a couple of changes of the default filters between python 3.0, 3.2 and 3.7. (Before 3.2, all DeprecationWarnings were shown, then none).

Since 3.7, it is configured by default as described in PEP565: Show all deprecation warnings that are raised by user code, ie code in the __main__ module, while testing environments like pytest and unittest issue something similar to

warnings.filterwarnings('default', DeprecationWarning)

to display all of them.

By raising to stacklevel=3 as proposed by @znicholls we are actually referring to the user code and should again raise by default unless it's further down in the package stack (ie. people make use of domestic_pathways which uses a deprecated pyam call, then it is not shown, unless one runs tests).

I think that is a sane compromise.

@coroa
Copy link
Collaborator Author

coroa commented Oct 26, 2020

With that change your snippet produces:

In [1]: import pandas as pd
   ...: import pyam
   ...:
   ...: TEST_DF = pd.DataFrame([
   ...:     ['model_a', 'scen_a', 'World', 'Primary Energy', 'EJ/yr', 1, 6.],
   ...:     ['model_a', 'scen_a', 'World', 'Primary Energy|Coal', 'EJ/yr', 0.5, 3],
   ...:     ['model_a', 'scen_b', 'World', 'Primary Energy', 'EJ/yr', 2, 7],
   ...: ],
   ...:     columns=pyam.IAMC_IDX + [2005, 2010],
   ...: )
   ...:
   ...: df = pyam.IamDataFrame(TEST_DF)
   ...: df.models()
pyam - INFO: Running in a notebook, setting `pyam` logging level to `logging.INFO` and adding stderr handler
<ipython-input-1-b2d2be267457>:13: DeprecationWarning: This method is deprecated and will be removed in future versions. Use the attribute `model` instead.
  df.models()
Out[1]:
0    model_a
Name: model, dtype: object

@danielhuppmann danielhuppmann mentioned this pull request Oct 28, 2020
@danielhuppmann
Copy link
Member

@coroa, thanks for the modification, this now works as I expect. Does it make sense to merge this as is or refactor the deprecation-warning as part of the discussion in #449?

@coroa
Copy link
Collaborator Author

coroa commented Oct 28, 2020

I'd merge as is. Logging initialization is a separate issue altogether

@danielhuppmann
Copy link
Member

ok, can you please rebase and add to the release notes?

@coroa coroa force-pushed the use-warning-module branch from 24475c8 to 0c32039 Compare October 29, 2020 18:37
@coroa
Copy link
Collaborator Author

coroa commented Oct 29, 2020

Done, i'd suggest squashing.

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

Thanks @coroa, looks good

@danielhuppmann danielhuppmann merged commit 2a569ff into IAMconsortium:master Oct 30, 2020
@coroa coroa deleted the use-warning-module branch November 2, 2020 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement logging Issues related to logging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants