Skip to content

Improve deprecation messages #1479

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

Closed
vfdev-5 opened this issue Nov 27, 2020 · 9 comments · Fixed by #1585
Closed

Improve deprecation messages #1479

vfdev-5 opened this issue Nov 27, 2020 · 9 comments · Fixed by #1585

Comments

@vfdev-5
Copy link
Contributor

vfdev-5 commented Nov 27, 2020

🚀 Feature

House keeping request to improve the codebase.
The idea is to 1) improve deprecation messages inspired from scikit-image: https://github.com/scikit-image/scikit-image/blob/87a8806cca7fb5366b6e5ddbe5e46364b44f90fe/skimage/_shared/utils.py#L15-L214

            f"{self.arg_name} argument is deprecated and will be removed "
            f"in version {self.changed_version}. To avoid this warning, "
            f"please do not use the {self.arg_name} argument. Please "
            f"see {func.__name__} documentation for more details.")

and 2) introduce similar decorators in our codebase in a minimalistic way.

@pillemer
Copy link

Hey, I'm pretty new to contributing and I think this issue is something I can help you with.
The CONTRIBUTING.md is super detailed(!) so I'm feeling confident I can follow those instructions.
I'd like a to ask for little more specific detail about what is needed in this issue and where to look for the messages that need to be modified. As I understand it, the formatting of the current messages needs to be more simialr to the the one quoted? (short concise f-strings.)

@vfdev-5
Copy link
Contributor Author

vfdev-5 commented Nov 30, 2020

Hi @pillemer sorry for delayed reply ! The idea is to make sure that current deprecation messages like

raise DeprecationWarning(
"ignite.contrib.engines.common.setup_any_logging is deprecated since 0.4.0. and will be remove in 0.6.0. "
"Please use instead: setup_tb_logging, setup_visdom_logging or setup_mlflow_logging etc."
)

express a similar message:

  • Aomething (e.g. function) is deprecated (we can keep the info since when it is deprecated)
  • And will be removed in version vX.Y.Z
  • To avoid this warning, please use something else (e.g. another function).

Currently, I found only one deprecation warning. Let's improve its message and for the others that I forgot.
We can not use f-strings as we still provide a support for python 3.5.
EDIT: we are not supporting python 3.5 since v0.5.0 => OK for the usage of f-strings (see #1496)

Feel free to ask other questions if needed...

@afzal442
Copy link
Contributor

We can not use f-strings as we still provide a support for python 3.5.

Hi, do you still provide support for python 3.5? Thanks.

@vfdev-5
Copy link
Contributor Author

vfdev-5 commented Dec 15, 2020

Hi, do you still provide support for python 3.5? Thanks.

@Afzal-Ind we just dropped the support of python 3.5 today.

@Devanshu24
Copy link
Contributor

Hi @vfdev-5 !
I would like to give it a shot, if it is okay with you and if no one else is already working on it :)

PS: I did not know if I was supposed to tag you, sorry if I wasn't supposed to do that

@vfdev-5
Copy link
Contributor Author

vfdev-5 commented Jan 21, 2021

@Devanshu24 sure, sounds good ! Currently, no one is working on this issue. Let me assign it to you such that other would know. Feel free to ask questions to detail what we would like to implement if needed and send a draft PR that we could improve. Let me know if it sounds good for you too. Thanks !

@Devanshu24
Copy link
Contributor

Sure sounds great, and thank you for the opportunity!
I'll get right to it. I will first find the current deprecation messages and understand the codebase a little, and will send a draft PR soon!

@Devanshu24
Copy link
Contributor

One small question, where would we want to place all these decorators?
As in, inside utils.py or maybe a separate file in itself?

@vfdev-5
Copy link
Contributor Author

vfdev-5 commented Jan 26, 2021

I think ignite.utils.py can be a good place for that. If needed we can come back to this point in the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants