Deprecation warning fixes - Related to issue #479

[DEKHTIARJonathan]

All the details related to this PR can be found in issue #479.

Please proof-read and test the PR before merging. I hope I didn't break anything in the process ;)

Thanks a lot,

Jonathan

[zsdonghao]

hi, i prefer to change all pre_layer back to layer.

[DEKHTIARJonathan]

@zsdonghao : damned, I just spent two hours correcting this :(

I do my best to re-modify all of this and then update this PR

[zsdonghao]

@DEKHTIARJonathan wait a moment, let me discuss with @lgarithm and @luomai first ..

[luomai]

@DEKHTIARJonathan Thanks for the PR. Really appreciate it.
I am discussing with @zsdonghao and @lgarithm to figure out the best way to resolve the layer vs prev_layer. The other deprecation fixes look good to us.

In the long term, providing these two key-based input parameters would confuse users. We prefer keeping either one of them. We are open for switching back to the layer. However, we need to discuss with other TL users to see if they have switched to prev_layer already, and they may complain why we change back.

This change was included TL 1.8 which is a maintenance release. TL 1.8 resolves many technical debts and we expected some of the API might breaks. We are very sorry about for your experience. At that time, we are also conservative about the release and 1.8 gone through a pre-release stage for public feedbacks.

Thanks again for your contribution.

[DEKHTIARJonathan]

@zsdonghao @luomai thanks a lot for your feedback, I am finishing this PR and fixing the errors with the tests. It will be easier to fix the library whatever decision you take afterward.

In anyway, I think it is of good practice to not "just removed" something, and first put a deprecation warning. My complaint wasn't about "why did you do it" but more about the way it is conducted. However, I must admit I don't really understand why it was changed and would be fine with a rollback on the name of this argument.

[lgarithm]

I think the first argument of Layer should be passed as positional argument instead of keyword argument (we should fix our code to follow that convention too), in that case, the name of the first argument (layer or prev_layer) becomes an internal concept, and it shouldn't appear at the caller.

[DEKHTIARJonathan]

Hi @luomai thanks a lot for your feedback, I really appreciate it.

I completely miss this fact... I think I can fix this point. I try to submit a new commit tonight or tomorrow solving this. Would this be fine to you ?

It may not require a lot of work to make this PR compatible with function/class calls without named arguments.

[DEKHTIARJonathan]

I have implemented a solution based on this simple idea, I am currently updating all my modifications.

I implemented a custom decorator that handle renaming for both functions and methods.
This has the advantages of just providing an alias to arguments, not adding any argument and not changing the order.

import sys
import functools
import warnings
import logging


def deprecated_alias(end_support_version, **aliases):
    def deco(f):

        @functools.wraps(f)
        def wrapper(*args, **kwargs):

            try:
                func_name = "{}.{}".format(args[0].__class__.__name__, f.__name__)
            except (NameError, IndexError):
                func_name = f.__name__

            rename_kwargs(
                kwargs,
                aliases,
                end_support_version,
                func_name
            )

            return f(*args, **kwargs)

        return wrapper

    return deco


def rename_kwargs(kwargs, aliases, end_support_version, func_name):

    for alias, new in aliases.items():

        if alias in kwargs:

            if new in kwargs:
                raise TypeError('{}() received both {} and {}'.format(func_name, alias, new))

            warnings.warn('{}() - {} is deprecated; use {}'.format(func_name, alias, new), DeprecationWarning)
            logging.warning("DeprecationWarning: {}(): "
                            "`{}` argument is deprecated and will be removed in version {}, "
                            "please change for `{}.`".format(func_name, alias, end_support_version, new))
            kwargs[new] = kwargs.pop(alias)
            

class MyClass(object):

    @deprecated_alias(object_id='id_object', end_support_version=1.6)
    def __init__(self, id_object):
        self.id = id_object
        
        print("I am object: %d" % self.id)
        sys.stdout.flush()

@deprecated_alias(name='username', end_support_version=1.6)        
def yolo(username):
    print("YOLLLOOOOOOO %s" % username)    
    sys.stdout.flush()

if __name__ == "__main__":

    object1 = MyClass(id_object=1234) ## Correct call with new argument name
    object2 = MyClass(object_id=1234) ## DeprecationWarning: call with old argument name
    
    print()
    
    yolo(username="John") ## Correct call with new argument name
    yolo(name="John")     ## DeprecationWarning: call with old argument name

Launch with the script: python main.py

Console Output:

I am object: 1234
WARNING:root:DeprecationWarning: MyClass.__init__(): `object_id` argument is deprecated and will be removed in version 1.6, please change for `id_object.`
I am object: 1234

YOLLLOOOOOOO John
WARNING:root:DeprecationWarning: yolo(): `name` argument is deprecated and will be removed in version 1.6, please change for `username.`
YOLLLOOOOOOO John

What are the benefits of this PR

In addition to handling the deprecation warning in a proper manner (thanks to your feedback), I have implemented a decorator that will ease a lot future deprecations and can be fixed in a minute.

I hope this solution will satisfy your requirements.

@zsdonghao && @luomai: Do you have any communication channel that I can join ? I would love to join the TL community and help you on the development.

[DEKHTIARJonathan]

With the commit 8ea85df I have finished implementing this idea.

You can now run this code seemlessly:

import tensorflow as tf
import tensorlayer as tl

x = tf.placeholder("float32", [None, 100])

input = tl.layers.InputLayer(x, name='in')

out1 = tl.layers.DenseLayer(input, 80, name='h1')
out2 = tl.layers.DenseLayer(prev_layer=input, n_units=80, name='h2')
out3 = tl.layers.DenseLayer(layer=input, n_units=80, name='h3')

Gives the following output:

[TL] InputLayer  in: (?, 100)
[TL] DenseLayer  h1: 80 identity
[TL] DenseLayer  h2: 80 identity
[TL] DeprecationWarning: DenseLayer.__init__(): `layer` argument is deprecated and will be removed in version 1.9, please change for `prev_layer.`
[TL] DenseLayer  h3: 80 identity

The new solution should address every situations ;)

[luomai]

Cool! The solution looks pretty elegant! Nice effort. :)

[zsdonghao]

@DEKHTIARJonathan @luomai @lgarithm lets join slack~~

[luomai]

@wagamamaz a friendly ping, in case you are also interested in joining the Slack channel.

[DEKHTIARJonathan]

@zsdonghao thanks a lot, I was already on slack. It is pretty inactive, is there any private channel where you discuss with the development team?

[zsdonghao]

hi, @DEKHTIARJonathan we have a private channel in slack , and we just invite you