-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add Deprecation Warnings #2321
Add Deprecation Warnings #2321
Conversation
src/python/turicreate/aggregate.py
Outdated
@@ -85,8 +85,11 @@ def COUNT(*args): | |||
|
|||
""" | |||
# arguments if any are ignored | |||
return ("__builtin__count__", [""]) | |||
if len(args) == 0: | |||
print('[WARNING] Passing parameter(s) to COUNT is deprecated. This functionality will be removed in ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using the warnings module for this stuff? Note there is a DeprecationWarning there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DeprecationWarning
is an exception. We don't want to raise an exception (i.e. error out) in these cases.
Using the warning module would probably be better. Although I think a lot of places in our code base currently just use print statements for warnings. We are using the warning module in a few places in the codebase. However we seem to be using logging.warning(...)
more. It not clear to me if one is better than the other.
@znation - do you have an option about using warnings.warn(...)
vs logging.warning(...)
? Since we're using it more, I think logging.warning(...)
might be the best option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also https://docs.python.org/2.7/library/logging.html#integration-with-the-warnings-module -- which would capture all warnings.warn
calls and redirect them to the logger. I think warnings.warn
is probably the "right" API for deprecating things in Python, using a DeprecationWarning as an argument to it, rather than raising one), but logging.warning
is probably the 2nd best thing to do. Raw print statements is not great since it's harder for users to redirect it (and to redirect it independently of their desired output, like progress tables).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly warnings.warn("Message", DeprecationWarning)
doesn't work in our codebase (the line is executed without error but nothing gets printed). That line does work fine in a standalone script. I suspect it has something to do with how we setup our logging. I'll investigate.
warnings.warn("message")
and logging.warn("message")
both work fine in our codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out this issue has nothing to do with our codebase. From the documentation for DeprecationWarning
:(ignored by default, unless triggered by code in __main__).
That's a very interesting decision, but it explains why it didn't trigger in our codebase but did in a standalone script.
So I don't think warnings.warn
with a DeprecationWarning
parameter is the right way to go. I'll update this PR to call warnings.warn
without a DeprecationWarning
parameter. @znation - sounds good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that sounds good to me, thanks! I am also surprised that deprecation warnings aren't on by default.
858a36d
to
a2c57e0
Compare
0acf520
to
22fbbe1
Compare
Internal test pass |
No description provided.