-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-22313][PYTHON] Mark/print deprecation warnings as DeprecationWarning for deprecated APIs #19535
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
Conversation
srowen
left a comment
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.
Seems OK to me if it's done consistently and actively helps developers see this info in Python
|
Let me probably open up a JIRA and link it. |
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.
python/pyspark/sql/functions.py
Outdated
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.
Here, I intendedly avoided *args and **kwargs to keep the argument signature printed in pydoc, help(...).
|
Test build #82906 has finished for PR 19535 at commit
|
|
Looks good at high level. |
felixcheung
left a comment
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.
LGTM, minor comment
python/pyspark/streaming/flume.py
Outdated
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.
for these, could you provide more information? link to the doc on deprecating DStream in python?
python/pyspark/streaming/kafka.py
Outdated
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.
ditto here
|
Thanks @felixcheung. Will update soon. |
f8a7d2b to
5ac0697
Compare
|
Test build #82956 has finished for PR 19535 at commit
|
|
Test build #82957 has finished for PR 19535 at commit
|
|
Thanks @srowen, @rxin and @felixcheung. |
|
Merged to master |
| .. note:: Deprecated in 2.3.0. Flume support is deprecated as of Spark 2.3.0. | ||
| See SPARK-22142. | ||
| """ | ||
| warnings.warn( |
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.
Seems warnings is not imported in this file?
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, it is not. Will make a followup after double checking other files too. Thank you.
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.
thank you :) It will be good to also check why master build does not fail since python should complain about it.
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.
Sure, I took a quick look and I think this one is actually not being tested and seems that's why .. will double check and take a closer look tonight (KST).
I have seen few mistakes about this so far and .. I am working on Python coverage BTW - https://issues.apache.org/jira/browse/SPARK-7721
Anyway, it was my stupid mistake. Thanks ..

What changes were proposed in this pull request?
This PR proposes to mark the existing warnings as
DeprecationWarningand print out warnings for deprecated functions.This could be actually useful for Spark app developers. I use (old) PyCharm and this IDE can detect this specific
DeprecationWarningin some cases:Before
After
For console usage,
DeprecationWarningis usually disabled (see https://docs.python.org/2/library/warnings.html#warning-categories and https://docs.python.org/3/library/warnings.html#warning-categories):so, it won't actually mess up the terminal much unless it is intended.
If this is intendedly enabled, it'd should as below:
These instances were found by:
How was this patch tested?
Manually tested.