Introduce @deprecated decorators for functions and enforce warnings as errors in tests

[RohitP2005]

Description

This pull request has implemented deprecation decorators as wrapper for a function's arguement and the fucntion itself and has added -W flag to ensure the deprecation warnings are treated as error

Related to: #2028

Type of change

• [x] New feature (non-breaking change that adds functionality)
• [x] Optimization (back-end change that speeds up the code)
• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] Deprecation (non-breaking change that deprecates functionality with warnings)

Key checklist:

• [x] No style issues: $ pre-commit run --all-files (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally)
• [x] All tests pass: $ python -m pytest -W error (or $ nox -s tests) (ensure warnings are treated as errors to catch deprecation notices)
• [x] The documentation builds: $ python -m pytest --doctest-plus src (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once using $ nox -s quick.

Further checks:

• [ ] Code is commented, particularly in hard-to-understand areas, including deprecation warnings and helper functions.
• [ ] Tests added for any new functionality or to prove that the deprecation warnings are effective.
• [x] Confirm that any borrowed code from Astropy is appropriately attributed according to its license.

[RohitP2005]

Previous Discussions : https://github.com/pybamm-team/PyBaMM/pull/4745#issuecomment-2590872142

[kratman]

Original issue request: "Replace existing deprecation warnings/errors with deprecation package so tests fail after a few releases."

[RohitP2005]

Okay , I misunderstood that the deprecation package was not required and we needed a pythonic way. Now i understand @kratman , Thankyou for the clarification .

I will just go ahead with the deprecation package with my next commit

Apologies for the confusion

[RohitP2005]

@kratman, I've implemented the deprecation package for four of the existing deprecation warnings. Could you please guide me on any potential improvements?

Regarding renamed and deprecated arguments, it seems the deprecation package doesn't natively handle these cases. Do you have any suggestions on how to approach this? I considered writing custom decorators to manage them. Does that sound like a good plan?

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.65%. Comparing base (1b84266) to head (71a5197). Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4757      +/-   ##
===========================================
- Coverage    98.57%   97.65%   -0.93%     
===========================================
  Files          304      304              
  Lines        23656    23672      +16     
===========================================
- Hits         23320    23117     -203     
- Misses         336      555     +219     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[kratman]

Does the package you use allow for tests to fail when the deprecation is old?

[RohitP2005]

Does the package you use allow for tests to fail when the deprecation is old?

No, the deprecation package does not natively support automatically failing tests when a deprecation is old

[RohitP2005]

Hey @agriyakhetarpal,

When I configured pytest to treat DeprecationWarnings as errors by removing the ignore::DeprecationWarning from the filterwarnings section, I encountered the following error. I’m unable to silence this specific warning and could use some guidance on how to resolve it.

ImportError while loading conftest '/home/maverick/Desktop/PyBaMM/conftest.py'.
conftest.py:3: in <module>
    import pybamm
src/pybamm/__init__.py:22: in <module>
    from .citations import Citations, citations, print_citations
src/pybamm/citations.py:271: in <module>
    citations = Citations()
src/pybamm/citations.py:36: in __init__
    self.read_citations()
src/pybamm/citations.py:68: in read_citations
    parse_file = import_optional_dependency("pybtex.database", "parse_file")
src/pybamm/util.py:358: in import_optional_dependency
    module = importlib.import_module(module_name)
venv/lib/python3.12/site-packages/pybtex/database/__init__.py:44: in <module>
    from pybtex.plugin import find_plugin
venv/lib/python3.12/site-packages/pybtex/plugin/__init__.py:26: in <module>
    import pkg_resources
venv/lib/python3.12/site-packages/pkg_resources/__init__.py:98: in <module>
    warnings.warn(
E   DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html```

[agriyakhetarpal]

Hey @agriyakhetarpal,

When I configured pytest to treat DeprecationWarnings as errors by removing the ignore::DeprecationWarning from the filterwarnings section, I encountered the following error. I’m unable to silence this specific warning and could use some guidance on how to resolve it.

ImportError while loading conftest '/home/maverick/Desktop/PyBaMM/conftest.py'.
conftest.py:3: in <module>
    import pybamm
src/pybamm/__init__.py:22: in <module>
    from .citations import Citations, citations, print_citations
src/pybamm/citations.py:271: in <module>
    citations = Citations()
src/pybamm/citations.py:36: in __init__
    self.read_citations()
src/pybamm/citations.py:68: in read_citations
    parse_file = import_optional_dependency("pybtex.database", "parse_file")
src/pybamm/util.py:358: in import_optional_dependency
    module = importlib.import_module(module_name)
venv/lib/python3.12/site-packages/pybtex/database/__init__.py:44: in <module>
    from pybtex.plugin import find_plugin
venv/lib/python3.12/site-packages/pybtex/plugin/__init__.py:26: in <module>
    import pkg_resources
venv/lib/python3.12/site-packages/pkg_resources/__init__.py:98: in <module>
    warnings.warn(
E   DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html```

I think this suggests that setuptools is missing in the virtual environment (we rely on it, as this fix https://bitbucket.org/pybtex-devs/pybtex/pull-requests/46 hasn't landed in a pybtex release yet as it is not maintained very actively). There has been a discussion about replacing it, but I don't think much progress has been made (#3648). It should work if you install setuptools and retry running pytest. I've triggered the CI workflows for you in the meantime.

[RohitP2005]

Hey @agriyakhetarpal, When I configured pytest to treat DeprecationWarnings as errors by removing the ignore::DeprecationWarning from the filterwarnings section, I encountered the following error. I’m unable to silence this specific warning and could use some guidance on how to resolve it.

ImportError while loading conftest '/home/maverick/Desktop/PyBaMM/conftest.py'.
conftest.py:3: in <module>
    import pybamm
src/pybamm/__init__.py:22: in <module>
    from .citations import Citations, citations, print_citations
src/pybamm/citations.py:271: in <module>
    citations = Citations()
src/pybamm/citations.py:36: in __init__
    self.read_citations()
src/pybamm/citations.py:68: in read_citations
    parse_file = import_optional_dependency("pybtex.database", "parse_file")
src/pybamm/util.py:358: in import_optional_dependency
    module = importlib.import_module(module_name)
venv/lib/python3.12/site-packages/pybtex/database/__init__.py:44: in <module>
    from pybtex.plugin import find_plugin
venv/lib/python3.12/site-packages/pybtex/plugin/__init__.py:26: in <module>
    import pkg_resources
venv/lib/python3.12/site-packages/pkg_resources/__init__.py:98: in <module>
    warnings.warn(
E   DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html```

I think this suggests that setuptools is missing in the virtual environment (we rely on it, as this fix https://bitbucket.org/pybtex-devs/pybtex/pull-requests/46 hasn't landed in a pybtex release yet as it is not maintained very actively). There has been a discussion about replacing it, but I don't think much progress has been made (#3648). It should work if you install setuptools and retry running pytest. I've triggered the CI workflows for you in the meantime.

yeah i tried it just now, and the pytest is still failling locally also

[agriyakhetarpal]

Thanks for trying it out, @RohitP2005. I think I have another approach. The idea is to ignore any external deprecation warnings outside of PyBaMM's codebase, but convert PyBaMM's warnings to errors.

Something like

[tool.pytest.ini_options]
filterwarnings = [
    "...", # rest of the options
    "error::DeprecationWarning:pybamm.*",
    "ignore::DeprecationWarning",
    "...", # and so on
]

should work here (haven't confirmed). Could you please try this?

[RohitP2005]

Thanks, @RohitP2005! Could you please resolve the conflicts here? It looks close to merging with not many more changes required.

Okay @agriyakhetarpal will address all those changes in next commit as requested.

[agriyakhetarpal]

I've reviewed the changes again. Sorry if you were adding any other changes during my review!

[RohitP2005]

Hey @kratman could u trigger the CIs for this one @agriyakhetarpal I think this PR is good to go now.

[RohitP2005]

@arjxn-py Thanks for initiating the CI's Can anyone help me with the pkg_resources is deprecated as an API
I tried silencing it and still it pops up Any possible fix for this

[RohitP2005]

Hey @agriyakhetarpal would be helpful if can come up with a solution for this , I tried everything I saw online to silence the pkg resources deprecation warnings

[RohitP2005]

I think there was one more warning underneath that you needed to fix. The suggestion I've left below makes the test suite run on my machine till completion. I'll note that you'll need to fix the warnings that are now being perceived as errors due to "error::DeprecationWarning:pybamm.*", in the configuration.

Also, could you please merge the latest changes from the main branch?

Thanks for the tip, I have commited the changes @agriyakhetarpal can u retrigger the CI

[agriyakhetarpal]

Done. Now, we'll need to fix the warnings as applicable, or skip them if they are difficult to fix and raise upstream issues as applicable.

[RohitP2005]

I think we can skip the existing deprecation warning and create a new issue to address them. I would like to work on it if that feels feasible.

I can see that pkg_resources.declare_namespace is still not being silenced despite your suggestion mentioned in this PR review.

This issue has already been discussed here.
Will look into a fix.

[agriyakhetarpal]

Yes, pybtex continues to give us issues, for example, see #3648.

[RohitP2005]

you can mark this PR as draft until pybtex is sorted !

Since the tests are failing as expected due to deprecation. how should i deal with them

[agriyakhetarpal]

Okay, after looking at the CI logs more closely, I'm not sure I follow, as the pybtex issue no longer shows up?

For the BPX tests, we should file an upstream issue about the Pydantic deprecation (or better, a PR fixing it) and ignore the warning in the filters here for the time being until it is fixed and makes it to a release.

For the deprecation warnings in the tests that are coming from PyBaMM's deprecated APIs, those are the actual ones we need to resolve. Usually, we deprecate methods/arguments/etc. by adding a DeprecationWarning, but we never remove the deprecated methods/arguments in the tests that might be using them. This PR has been a step forward towards catching such uses, so that they become hard errors instead of showing up as warnings. In some tests, we are catching and thereby testing if the appropriate DeprecationWarnings are raised when calling a deprecated method/argument – these are the ones that are useful, and should be kept.

I hope this is clear!

[RohitP2005]

Okay, after looking at the CI logs more closely, I'm not sure I follow, as the pybtex issue no longer shows up?

For the BPX tests, we should file an upstream issue about the Pydantic deprecation (or better, a PR fixing it) and ignore the warning in the filters here for the time being until it is fixed and makes it to a release.

For the deprecation warnings in the tests that are coming from PyBaMM's deprecated APIs, those are the actual ones we need to resolve. Usually, we deprecate methods/arguments/etc. by adding a DeprecationWarning, but we never remove the deprecated methods/arguments in the tests that might be using them. This PR has been a step forward towards catching such uses, so that they become hard errors instead of showing up as warnings. In some tests, we are catching and thereby testing if the appropriate DeprecationWarnings are raised when calling a deprecated method/argument – these are the ones that are useful, and should be kept.

I hope this is clear!

Understood very well That clarifies everything

[RohitP2005]

Hey @agriyakhetarpal i fixed the tests with deprecated functions but i need help with the test_bpx.py file

[RohitP2005]

Hey @agriyakhetarpal i fixed the tests with deprecated functions but i need help with the test_bpx.py file

It has been currently ignored in the pytest

[RohitP2005]

Hey @agriyakhetarpal this issue have been here for a while, I have made the changes as requested . Kindly trigger the CI so that we can merge it asap

[RohitP2005]

@agriyakhetarpal I just need a little help, the test past successfully locally in my machine. I tried silencing and the doctest also run without fails .

is there smth i am missing because of which the CI are failing

[agriyakhetarpal]

It fails on my end, could you please check if pytest is picking up the configuration from pyproject.toml?

[RohitP2005]

@agriyakhetarpal I just need to confirm the CI with this commit, kindly trigger the CIs.

[RohitP2005]

Hey @agriyakhetarpal I just bumped down the version of setuptools to handle the deprecated_pkgresources warning, will that be a feasible solution. ?