Managing Deprecation using decorators

[Devanshu24]

This is a very stripped down version of the code, I have not written any tests yet. This is primarily for me to check if I am going in the correct direction. So please do tell everything that needs improvement/changing.

Fixes #1479

Description:
Implemented till now

• Make functions deprecated using the @deprecated decorator
• Add arguments to the @deprecated decorator to customize it for each function
• The deprecation messages also reflect in the documentation (written in Sphinx compatible format)

Check list:

• New tests are added (if a new feature is added)
• New doc strings: description and/or example code are in RST format
• Documentation is updated (if required)

Thank you to @ydcjeff for giving the idea to add the version update information :)

[sdesrozis]

@Devanshu24 Thank you, it looks very good to me!

[Devanshu24]

Thank you for your review @sdesrozis!
So since no one suggested any changes I hope this is a good way to proceed.
I plan to add the following decorators (and their tests):

• @deprecated (the one currently shown)
• @version_added
• @version_changed
• @change_default_value changes the default value of an argument
• @remove_arg
• @deprecate_kwarg

I found these from the link @vfdev-5 gave in #1479 (this one to be precise) and little googling for similar stuff. However, I am not sure which of these would actually be helpful. I am more than happy to do all of them but I felt that if there is even one of them which will never be used it will just be some dead code.

So I am looking for your suggestions :)

[vfdev-5]

Thanks for the suggestions @Devanshu24 !
Let's start with @deprecated, as for the others let's see a bit later. Personnally, I do not have any strong opnion about other decorators.

[sdesrozis]

I agree @vfdev-5 about others decorators. Let's see first using @deprecated :) and we keep in mind other decorators !

[Devanshu24]

I have made some minimal changes to the @deprecated function to cover some edge cases. I have also added tests to test most of the use-cases (I hope nothing important has been left out :) )

If there are any changes that need to be made please let me know :)

PS: I have not removed the DeprecationWarning from the setup_any_logging function as I was not sure what to enter in the body of the function.

[sdesrozis]

It looks good to me !! @Devanshu24 Thank you !

[vfdev-5]

@sdesrozis should we also use in the code this decorator ?

[Devanshu24]

@vfdev-5 Any chages/additions needed?

Also if you could guide me on this :)

PS: I have not removed the DeprecationWarning from the setup_any_logging function as I was not sure what to enter in the body of the function.

[vfdev-5]

@Devanshu24 I think we should update setup_any_logging and use your decorator to show an example of usage.

[Devanshu24]

@Devanshu24 I think we should update setup_any_logging and use your decorator to show an example of usage.

Okay, and in the function body should I just add a pass statement?

EDIT: As the current one with raise DeprecationWarning() is actually causing an exception

[vfdev-5]

@Devanshu24 maybe we can configure the decorator such that it could either raise DeprecationWarning or use warning.warn ?

[Devanshu24]

@Devanshu24 maybe we can configure the decorator such that it could either raise DeprecationWarning or use warning.warn ?

Oh okay, I thought that as long as the function is still in the deprecated state (i.e. not yet removed) we wouldn't want the code to cause an exception and interrupt the program.
If you say so I could also add an option that raises an Exception?
So the options would be,

• No Exception, No Warning
• Just Warning, No Exception
• Raise Exception

PS: I learnt that raise DeprecationWarning() causes an exception from here

[vfdev-5]

Well, you are right that usually we do not raise exceptions on deprecated functions. It actually may depend to security criticity of the function (for example, see yaml.load: yaml/pyyaml#292). In our case, usage of setup_any_logging is not that critical, we just removed the implementation, so showing a warning and doing nothing is not an option for our code neither...

A bit details for your table, if function is decorated with deprecated:

• Deprecation in docs, Just Warning, No Exception
• Deprecation in docs, Raise Exception
• No Exception, No Warning (= no decorator, right?)

[Devanshu24]

Well, you are right that usually we do not raise exceptions on deprecated functions. It actually may depend to security criticity of the function (for example, see yaml.load: yaml/pyyaml#292). In our case, usage of setup_any_logging is not that critical, we just removed the implementation, so showing a warning and doing nothing is not an option for our code neither...

Oh okay, got to learn something new, thanks! :D

A bit details for your table, if function is decorated with deprecated:

• Deprecation in docs, Just Warning, No Exception
• Deprecation in docs, Raise Exception
• No Exception, No Warning (= no decorator, right?)

Yeah right, that makes sense, I thought that maybe we would want to have a case in which we just say it is deprecated in the docs, but not raise any warnings or exceptions. But your point makes more sense

I'll make the edits, and now we will have just two options:

• Deprecation in docs, Just Warning, No Exception
• Deprecation in docs, Raise Exception

[Devanshu24]

I am getting the following error in the unit tests and during deploying the docs

NameError: name 'TextIO' is not defined

I think the addition was made in #1601, to be precise in the line

ignite/ignite/utils.py

Line 71 in c25ce51

stream: Optional[TextIO] = None,

I haven't understood the error yet, thought I would ask if one of you know what it is, if not I'll start looking :)

[vfdev-5]

Oh, this looks like a bug. @sdesrozis any ideas ?

@Devanshu24 at which moment exactly do you see that, on which command ?

[Devanshu24]

Okay I figured it. It was a small thing, TextIO wasn't imported from typing

[vfdev-5]

Okay I figured it. It was a small thing, TextIO wasn't imported from typing

Normally, it should be imported here :

ignite/ignite/utils.py

Line 5 in c25ce51

from typing import Any, Callable, Optional, TextIO, Tuple, Type, Union, cast

[Devanshu24]

@Devanshu24 at which moment exactly do you see that, on which command ?

pytest tests/ignite/test_utils.py -s -vvv -k test_deprecated

and inside the docs folder with the following command

make html

---

Although after importing it both the commands work as expected :)

[Devanshu24]

I am not sure why the mypy test keeps failing, as they are working locally for me.
Trying to debug what is happening

---

Output of running it locally

$ mypy --config-file mypy.ini
Success: no issues found in 86 source files

[vfdev-5]

@Devanshu24 are using the same version ?

[Devanshu24]

@Devanshu24 are using the same version ?

It is the same as the one on the CI i.e. mypy 0.800
Although 2d49a48 fixes the mypy errors, the changes made in that commit pass tests on CI and also does not interfere with the local tests (which were already passing)

[Devanshu24]

I think all tests are now passing (except codecov, but the uncovered line will be uncovered)
Please have a look at your convenience :)

[vfdev-5]

Thanks for the updates @Devanshu24 !
Few other comments to address, please

[Devanshu24]

@vfdev-5 mypy is again failing on CI. It still passes locally though (EDIT: Can you also test is locally)
It is because of the incorrect placement of # type: ignore

[vfdev-5]

@Devanshu24 I have to check that locally from my side.

@gruebel do you have an idea about why mypy is failing ?

[Devanshu24]

Okayy I think I get it, the Python3.8 CI is still passing, which is my local python version too
There is some issue with 3.7 and 3.6

[gruebel]

@vfdev-5 I will take a look today, I also left some comments on the typing.

[vfdev-5]

Thanks a lot @gruebel !

[gruebel]

So, I digged into the problem it seems like to be an issue with mypy and how it handles multiline functions/decorators. The decorator belongs to the function, so mypy gets messed up and blames the first line. Interestingly it was fixed for Python 3.8, but not lower python/mypy#3251.

So, to make mypy happy it needs to be like @Devanshu24 implemented it before

@deprecated(
    "0.4.0",  # type: ignore
    "0.6.0",
    ["Please use instead: setup_tb_logging, setup_visdom_logging or setup_mlflow_logging etc."],
    raise_exception=True,
)
def setup_any_logging(logger, logger_module, trainer, optimizers, evaluators, log_every_iters) -> None:
    pass

or just add some type hints and then you can remove the # type: ignore part 😄

@deprecated(
    "0.4.0",
    "0.6.0",
    ["Please use instead: setup_tb_logging, setup_visdom_logging or setup_mlflow_logging etc."],
    raise_exception=True,
)
def setup_any_logging(
    logger: BaseLogger,
    logger_module: Any,
    trainer: Engine,
    optimizers: Optional[Union[Optimizer, Dict[str, Optimizer], Dict[None, Optimizer]]],
    evaluators: Optional[Union[Engine, Dict[str, Engine]]],
    log_every_iters: int,
) -> None:
    pass

I would prefer just to add the types, which approach do you prefer @vfdev-5 ?

[vfdev-5]

Thank a lot @gruebel for investigating the issue 💪 !
Yes, I also prefer the second solution with type hints as you suggested.
@Devanshu24 could you please apply those changes .

[Devanshu24]

Thanks @gruebel !
Sure @vfdev-5 will get them done today!

[Devanshu24]

Made the suggested changes, please have a look at your convenience :)

[vfdev-5]

Great work @Devanshu24 ! Thanks !