-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-33002][PYTHON] Remove non-API annotations. #29879
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
99e5074 to
d996c6e
Compare
|
Test build #129137 has finished for PR 29879 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
d996c6e to
037acdd
Compare
|
Test build #129253 has finished for PR 29879 at commit
|
|
I think it is as much as we can remove here. |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #129263 has finished for PR 29879 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Retest this please |
|
Test build #129300 has finished for PR 29879 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Test build #129361 has finished for PR 29879 at commit
|
|
Retest this please |
|
Test build #129364 has finished for PR 29879 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
HyukjinKwon
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.
Should we remove python/join.pyi, python/rddsampler.pyi and python/shell.pyi too?
LGTM otherwise.
I considered that but overall I'd prefer to keep it as-is:
I don't have strong opinion about it. However, same as join, it is stable with meaningful annotations and never marked as internal (I used it once or twice directly, so I suspect that others might as well). So indicates that it will cause serious maintenance overhead or is just a deadweight.
Yes, that's an omission. |
|
Hmm, but technically |
|
Test build #129438 has finished for PR 29879 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
Fair point. If you feel strong about these, I'll be happy to remove them. But I think we need a bit precise criteria of inclusion / exclusion in a long run. In general the most important factors are:
(I am primarily thinking about SPARK-33003 here) |
|
I personally tend to agree with what you listed up. However, one concern is that it doesn't work well out of the box when the criteria becomes verbose especially for new contributors given my experience - the same criteria is interpreted differently often. So I ended up with focusing on simplifying it unless it's required. Also, one more thing is that I would like to be explicit what we expose and hide as APIs. Something has been internal so it was changed but users ended up with complaining - this happens not very often but not so rarely. I would like to avoid this kind of cost. To sum up, we can keep |
|
Thanks @zero323. |
Makes sense. I am still thinking about this from a perspective of the main contributor :)
These two have rather limited scope so we're good for now. I didn't to make merge test, but if it becomes a problem later, we can always restore things ‒ stability works both ways here..
Sounds good. |
|
Test build #129499 has finished for PR 29879 at commit
|
HyukjinKwon
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
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Merged to master. |
|
Thanks @HyukjinKwon! |
What changes were proposed in this pull request?
This PR:
__init__.pyifiles, if no annotations, beyond exports, are present.Why are the changes needed?
Primarily to reduce maintenance overhead and as requested in the comments to #29591
Does this PR introduce any user-facing change?
No
How was this patch tested?
Existing tests and additional MyPy checks: