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

RohitP2005 · 2025-01-14T19:55:31Z

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

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

Key checklist:

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)
All tests pass: $ python -m pytest -W error (or $ nox -s tests) (ensure warnings are treated as errors to catch deprecation notices)
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.
Confirm that any borrowed code from Astropy is appropriately attributed according to its license.

RohitP2005 · 2025-01-14T19:55:59Z

Previous Discussions : #4745 (comment)

utils/exceptions.py

src/pybamm/simulation.py

src/pybamm/solvers/base_solver.py

kratman

I think one of the main reasons for doing this is to track the date of the deprecation warnings. This does not seem to be in these changes.

Realistically the warnings work as-is, but we want to automate the removal of them. Right now I use git-blame to remove old warnings every 6 months or so. The extra decorators and such are just additional overhead unless we use it for something better than the current warnings

kratman · 2025-01-14T20:08:11Z

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

RohitP2005 · 2025-01-14T20:17:41Z

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 · 2025-01-16T20:06:17Z

@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 · 2025-01-16T20:18:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

kratman · 2025-01-17T14:15:37Z

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

src/pybamm/simulation.py

RohitP2005 · 2025-01-20T03:16:18Z

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

agriyakhetarpal

I think I agree with the general idea here, and the @deprecated decorator looks neat. If there's no way to make the deprecated functions fail directly, that's fine – we should proceed with your suggestion in the PR description:

has added -W flag to ensure the deprecation warnings are treated as an error

I don't see this change here, but yes, please feel free to configure pytest with this, as the deprecation warnings would then be caught in the tests if they use it. Then, you can fix any pending deprecation warnings unrelated to your changes that show up in the test suite. If there are a bunch of them and they are not straightforward to fix, it is also okay to filter them using the filterwarnings configuration option and open a new issue about removing them to work on later.

See also: https://docs.pytest.org/en/stable/how-to/capture-warnings.html#controlling-warnings

Regarding deprecating a specific argument in a function, yes, adding a custom decorator for this sounds great. If you think it will require a bit of experimentation, I'm happy to help review it in a follow-up PR (you may edit the PR description to mark this PR as related to #2028, but not fixing it). Limiting the scope of this PR sounds good to me to get it in.

Thanks!

RohitP2005 · 2025-03-13T17:21:34Z

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 · 2025-03-13T19:35:08Z

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 · 2025-03-14T16:51:56Z

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 · 2025-03-26T09:31:28Z

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

RohitP2005 · 2025-03-26T09:31:50Z

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

agriyakhetarpal

Thanks, @RohitP2005! Just a few more comments to go here. Also, there's another warning in the doctests that we need to ignore, see: https://github.com/pybamm-team/PyBaMM/actions/runs/14079804552/job/39430386846?pr=4757

src/pybamm/util.py

agriyakhetarpal · 2025-03-26T11:21:39Z

tests/unit/test_expression_tree/test_symbol.py

-    def test_symbol_auxiliary_domains(self):
-        a = pybamm.Symbol(
-            "a",
-            domain="test",
-            auxiliary_domains={
-                "secondary": "sec",
-                "tertiary": "tert",
-                "quaternary": "quat",
-            },
-        )
-        assert a.domain == ["test"]
-        assert a.secondary_domain == ["sec"]
-        assert a.tertiary_domain == ["tert"]
-        assert a.tertiary_domain == ["tert"]
-        assert a.quaternary_domain == ["quat"]
-        assert a.domains == {
-            "primary": ["test"],
-            "secondary": ["sec"],
-            "tertiary": ["tert"],
-            "quaternary": ["quat"],
-        }
-
-        a = pybamm.Symbol("a", domain=["t", "e", "s"])
-        assert a.domain == ["t", "e", "s"]
-        with pytest.raises(TypeError):
-            a = pybamm.Symbol("a", domain=1)
-        b = pybamm.Symbol("b", domain="test sec")
-        with pytest.raises(pybamm.DomainError, match="All domains must be different"):
-            b.domains = {"primary": "test", "secondary": "test"}
-        with pytest.raises(pybamm.DomainError, match="All domains must be different"):
-            b = pybamm.Symbol(
-                "b",
-                domain="test",
-                auxiliary_domains={"secondary": ["test sec"], "tertiary": ["test sec"]},
-            )
-
-        with pytest.raises(NotImplementedError, match="auxiliary_domains"):
-            a.auxiliary_domains
-


May I know why this test was removed?

@RohitP2005, this conversation is still relevant – as I'm not sure why the test was removed. It should be reinstated, or we need a valid reason for why it should be removed.

Again, this conversation is still relevant – I'd like to know why we need to remove this test?

tests/unit/test_simulation.py

tests/unit/test_solvers/test_casadi_solver.py

RohitP2005 · 2025-04-09T21:21:31Z

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 · 2025-04-10T13:16:32Z

@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 · 2025-04-11T12:39:28Z

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

RohitP2005 · 2025-04-12T21:18:55Z

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

RohitP2005 · 2025-04-13T06:59:51Z

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

agriyakhetarpal · 2025-04-13T11:21:55Z

Yes, I'm okay with it if you can't ignore the warning in any other way. Thanks!

RohitP2005 · 2025-04-13T18:04:46Z

@agriyakhetarpal i guess the PR is ready to merge, Once the PR completes the CI

agriyakhetarpal

Thanks, @RohitP2005. This is inching closer to a merge now. I still have a few more comments:

agriyakhetarpal · 2025-04-13T18:06:15Z

pyproject.toml

    "-nauto",
    "-vra",
+    "-ra",
+    "--showlocals",


This conversation is still relevant, as I'm not sure why we need to include this option with this PR – is it related to the changes? It's useful, but if it doesn't need to be included for the main changes here, please consider adding it in another PR.

agriyakhetarpal · 2025-04-13T18:07:54Z

pyproject.toml

+    "--showlocals",
    "--strict-config",
    "--strict-markers",
+    "--ignore=tests/unit/test_parameters/test_bpx.py",


Rather than ignoring the file completely, let's skip/xfail the relevant tests in that file. As I previously mentioned: if you can't address/capture the warnings coming out of them, please open an issue in the BPX repo and link it here.

agriyakhetarpal · 2025-04-13T18:09:22Z

tests/unit/test_expression_tree/test_symbol.py

-    def test_symbol_auxiliary_domains(self):
-        a = pybamm.Symbol(
-            "a",
-            domain="test",
-            auxiliary_domains={
-                "secondary": "sec",
-                "tertiary": "tert",
-                "quaternary": "quat",
-            },
-        )
-        assert a.domain == ["test"]
-        assert a.secondary_domain == ["sec"]
-        assert a.tertiary_domain == ["tert"]
-        assert a.tertiary_domain == ["tert"]
-        assert a.quaternary_domain == ["quat"]
-        assert a.domains == {
-            "primary": ["test"],
-            "secondary": ["sec"],
-            "tertiary": ["tert"],
-            "quaternary": ["quat"],
-        }
-
-        a = pybamm.Symbol("a", domain=["t", "e", "s"])
-        assert a.domain == ["t", "e", "s"]
-        with pytest.raises(TypeError):
-            a = pybamm.Symbol("a", domain=1)
-        b = pybamm.Symbol("b", domain="test sec")
-        with pytest.raises(pybamm.DomainError, match="All domains must be different"):
-            b.domains = {"primary": "test", "secondary": "test"}
-        with pytest.raises(pybamm.DomainError, match="All domains must be different"):
-            b = pybamm.Symbol(
-                "b",
-                domain="test",
-                auxiliary_domains={"secondary": ["test sec"], "tertiary": ["test sec"]},
-            )
-
-        with pytest.raises(NotImplementedError, match="auxiliary_domains"):
-            a.auxiliary_domains
-


Again, this conversation is still relevant – I'd like to know why we need to remove this test?

RohitP2005 · 2025-04-13T18:39:55Z

@agriyakhetarpal

I used addopts = [ "-nauto", "-vra", "-ra", "--showlocals" to debug the failing test and forgot to remove them, thanks for pointing it out
I will try using the @pytest.mark.xfail decorators to suppress the test, if it persists , i will open a issue in BPX
Since the auxilary domains are deprecated I thought the test are not required, maybe i will rewrite it to test for deprecation warning in case of use of auxilary domainx

RohitP2005 · 2025-04-13T19:43:48Z

@agriyakhetarpal does the points sum up requested changes

RohitP2005 · 2025-04-18T14:25:31Z

@agriyakhetarpal I have rewritten tests for auxiliary domains
And I think that should cover the codecov test coverage

RohitP2005 requested review from a team, DrSOKane, MarcBerliner, Saransh-cpp, TomTranter, agriyakhetarpal, arjxn-py, brosaplanella, kratman, martinjrobins, rtimms and valentinsulzer as code owners January 14, 2025 19:55

kratman reviewed Jan 14, 2025

View reviewed changes

utils/exceptions.py Outdated Show resolved Hide resolved

kratman reviewed Jan 14, 2025

View reviewed changes

src/pybamm/simulation.py Outdated Show resolved Hide resolved

kratman reviewed Jan 14, 2025

View reviewed changes

src/pybamm/solvers/base_solver.py Outdated Show resolved Hide resolved

kratman requested changes Jan 14, 2025

View reviewed changes

Deprecation for deprecated fucntions

ff6a817

RohitP2005 force-pushed the issue#2028 branch from 5024850 to ff6a817 Compare January 16, 2025 19:55

style: pre-commit fixes

7f34a0b

prady0t suggested changes Jan 19, 2025

View reviewed changes

src/pybamm/simulation.py Outdated Show resolved Hide resolved

src/pybamm/simulation.py Outdated Show resolved Hide resolved

RohitP2005 and others added 2 commits February 2, 2025 19:08

Merge branch 'pybamm-team:develop' into issue#2028

3da7140

Removed previous deprecation warnings

3a15fe8

agriyakhetarpal requested changes Feb 2, 2025

View reviewed changes

agriyakhetarpal mentioned this pull request Mar 13, 2025

Switch to an active citations parser dependency to replace pybtex #3648

Closed

fixed deprecated tests

422029a

agriyakhetarpal reviewed Mar 26, 2025

View reviewed changes

RohitP2005 added 3 commits April 10, 2025 01:44

Merge branch 'pybamm-team:develop' into issue#2028

6e2eb5a

test coverage for deprecate warnings

840b696

Added _set_paramters function in the test

194c657

Merge branch 'develop' into issue#2028

41e58e3

RohitP2005 added 2 commits April 13, 2025 02:37

Unit test fixed

a565419

doctests fix

0bf9abf

Updated setuptools version

6a63a05

Merge branch 'develop' into issue#2028

444cf08

agriyakhetarpal requested changes Apr 13, 2025

View reviewed changes

RohitP2005 added 2 commits April 18, 2025 19:54

Merge branch 'pybamm-team:develop' into issue#2028

6db0714

added test auxiliary domains

2e8d684

Merge branch 'pybamm-team:develop' into issue#2028

71a5197

Uh oh!

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

RohitP2005 commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Key checklist:

Further checks:

Uh oh!

RohitP2005 commented Jan 14, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kratman left a comment

Choose a reason for hiding this comment

Uh oh!

kratman commented Jan 14, 2025

Uh oh!

RohitP2005 commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RohitP2005 commented Jan 16, 2025

Uh oh!

codecov bot commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kratman commented Jan 17, 2025

Uh oh!

Uh oh!

Uh oh!

RohitP2005 commented Jan 20, 2025

Uh oh!

agriyakhetarpal left a comment

Choose a reason for hiding this comment

Uh oh!

RohitP2005 commented Mar 13, 2025

Uh oh!

agriyakhetarpal commented Mar 13, 2025

Uh oh!

RohitP2005 commented Mar 14, 2025

Uh oh!

RohitP2005 commented Mar 26, 2025

Uh oh!

RohitP2005 commented Mar 26, 2025

Uh oh!

agriyakhetarpal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

agriyakhetarpal Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

agriyakhetarpal Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

agriyakhetarpal Apr 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RohitP2005 commented Apr 9, 2025

Uh oh!

RohitP2005 commented Apr 10, 2025

Uh oh!

agriyakhetarpal commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RohitP2005 commented Apr 12, 2025

Uh oh!

RohitP2005 commented Apr 13, 2025

Uh oh!

agriyakhetarpal commented Apr 13, 2025

RohitP2005 commented Jan 14, 2025 •

edited

Loading

RohitP2005 commented Jan 14, 2025 •

edited

Loading

codecov bot commented Jan 16, 2025 •

edited

Loading

agriyakhetarpal commented Apr 11, 2025 •

edited

Loading