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

Deprecate passing positional arguments to Parameters #737

Closed
wants to merge 7 commits into from

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Apr 12, 2023

Following the discussion in #730 I am opening this PR for the other maintainers to check how they feel about this change. This PR deprecates passing positional arguments to all the Parameters. I found this very handy decorator in xarray (that they adapted from scikit-learn) here https://github.com/pydata/xarray/blob/7c7e3838685bb49a38303c9d8a91daf62b4c0ebf/xarray/util/deprecation_helpers.py#L44.

I have also made the changes required in numbergen and the whole test suite to avoid the warnings. It'd be interesting to see how much work would be required to update Panel and HoloViews to have keyword arguments only, I suspect it would actually be less than what was required in the test suite.

Copy link
Member

@hoxbro hoxbro left a comment

Choose a reason for hiding this comment

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

I think we should create a new file _warnings.py or similar with contains the _deprecate_positional_args function.


warnings.warn(
f"Passing '{extra_args}' as positional argument(s) to 'param.{name}' "
"was deprecated, please pass them as keyword arguments.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"was deprecated, please pass them as keyword arguments.",
"is deprecated and will stop working in Param X.X. Please pass them as keyword arguments.",

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes not mentioning the exact Param release is intentional. I think Param 2.0 took a long time in the making because some changes were announced to happen specifically in Param 2.0 and it took a long time to happen. Oldest mention I can find on Github is in #73 (comment) and it is 8 years old! cc @jlstevens and it's from him and it's kind of funny :)

In practice what I envision is that this would emit a DeprecationWarning and about 3 to 6 months before the change (in Param 3.0 I guess) the warning emitted would be a FutureWarning so that all users get it.

Copy link
Member

Choose a reason for hiding this comment

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

Most people would not know that 2.0 took a long time and will just install the package.

And my feeling is that most people are lazy, and without a real deadline, there is no urgency and will therefore postpone it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Most people would not know that 2.0 took a long time and will just install the package.

I agree.

Simon's suggestion also seems sensible: giving the relevant param version in the warning.

Oldest mention I can find on Github is in #73 and it is 8 years old!

But is 8 years enough? :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Here are a couple counter-arguments.

I have been bitten enough by having to support Python 2 (syntax, test suite, etc.) for such a long time, because it was somehow announced, that I would now be very careful with announcing anything concrete. To me, announcing something is deprecated and programmatically is the most important piece of information. Having the version in the warning message also sets a precedent, making it more likely to have to add it to other warnings, reinforcing the probability for us to tie our hands more than we would like to, while I believe not providing much value to Param users.

Yes, people are lazy, and they usually know it's a bad thing! I would never complain to a maintainer for a breaking change, for which a warning was emitted for a long period of time, I'll just blame myself for being too lazy. All the more so when that breaking change happens in a major release, which we would do here.

I would not be so picky if there was not history, but I bet that if asked 8 years ago, none of the Param maintainers would have guessed that it would have taken so long for the release to happen (it's not over by the way, who knows how long it is going to take for it to really happen!).

Comment on lines 94 to 95
DeprecationWarning,
stacklevel=2,
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be beneficial to have a custom warning here. This will make it possible for our other project to raise the warning as an error in our test suite.

And have you checked if the stacklevel is correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep stacklevel is correct, I ran the test suite and got correct warnings (lots of them!).

I think it would be beneficial to have a custom warning here. This will make it possible for our other project to raise the warning as an error in our test suite.

Did you already figure out how to run a test suite and make it fail on some specific warnings only?

Copy link
Member

Choose a reason for hiding this comment

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

Yep stacklevel is correct, I ran the test suite and got correct warnings (lots of them!).

Sounds good, and this is also the case for packages outside Param?

Did you already figure out how to run a test suite and make it fail on some specific warnings only?

It is the same syntax as ignoring a file in filterwarnings. See here. But I can see that there is the possibility to reference the "path" with something like this error::DeprecationWarning:param.parameterized.

Though, this will raise errors for all DeprecationWarnings. By having custom warnings, we will have finer control.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, and this is also the case for packages outside Param?

Tested it in the context of other packages and it looked fine.

It is the same syntax as ignoring a file in filterwarnings. See here. But I can see that there is the possibility to reference the "path" with something like this error::DeprecationWarning:param.parameterized.

Great, I've added a ParamDeprecationWarning class, being a subclass of DeprecationWarning. The test suite fails now if this warning is emitted. I noticed that unfortunately the examples tests (I have just added that in this PR) don't fail when this warning is emitted, I presume this has to do with nbval :/

@hoxbro
Copy link
Member

hoxbro commented Apr 13, 2023

Have you tested whether _deprecate_positional_args will have any significant cost concerning execution time?

@maximlt
Copy link
Member Author

maximlt commented Apr 13, 2023

I think we should create a new file _warnings.py or similar with contains the _deprecate_positional_args function.

Yep there's an issue on refactoring Param generally, which I would prefer to do at once in another PR. For now I just make sure any new symbol that isn't meant to be public starts with an _.

Have you tested whether _deprecate_positional_args will have any significant cost concerning execution time?

I just did a very simple check, timing running the test suite on main and on this branch, I couldn't see any significant difference. That is a pretty poor benchmark though. I never looked into how to set up a performance test suite. There's of course an issue for that :) #165.

@hoxbro
Copy link
Member

hoxbro commented Apr 13, 2023

Yep there's an issue on refactoring Param generally, which I would prefer to do at once in another PR. For now I just make sure any new symbol that isn't meant to be public starts with an _.

In a big refactor PR, a lot of code will be moved around, making it hard to judge that if something like _warnings.py is the best filename. This PR is adding this as a new feature, so I don't see any problem with putting it in the "correct" location to start with.

@maximlt
Copy link
Member Author

maximlt commented Apr 14, 2023

In a big refactor PR, a lot of code will be moved around, making it hard to judge that if something like _warnings.py is the best filename. This PR is adding this as a new feature, so I don't see any problem with putting it in the "correct" location to start with.

Really I'd prefer to keep that for a later refactor. That will probably end up in a _utils module.

@maximlt
Copy link
Member Author

maximlt commented Apr 14, 2023

I had forgotten to update String, which I have fixed now. And I have also updated the docs. This is ready for another round of review.

@maximlt
Copy link
Member Author

maximlt commented May 23, 2023

Superseded by #751

@maximlt maximlt closed this May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants