-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-15790][MLlib] Audit @Since annotations in ML #17314
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
|
@ehsun7b close this please |
|
Why? @srowen |
|
This looks like it is from some other project. What is Openet labs? See http://spark.apache.org/contributing.html |
|
It's our fork of Apache Spark. |
|
OK. Is this intended as a pull request for upstream Spark? (We get a couple accidental PRs a week where someone intended to open a PR against their fork) Reformat the title and explain a bit more about the change. |
|
Yes it is for upstream Spark. Our team is going to contribute on fixing Apache Spark bugs or fixing open JIRA tickets for documentations. |
|
Should I re-open the PR? |
|
I updated the title and provided more details about the change. |
|
Please fix the title -- see the link I sent you |
| import org.apache.spark.sql.{DataFrame, Dataset} | ||
|
|
||
| @Since("2.0.0") | ||
| private[r] class AFTSurvivalRegressionWrapper private ( |
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 and many files below are private though?
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.
Private classes must not have the annotation?
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.
No real point because the annotations are for end users and these don't appear in APIs or docs
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.
OK,
We have added the annotations already and the versions are very precise because we have developed a tool to find the exact version a class appeared in, for the first time.
Should we remove the annotations from private classes?
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.
Hm, I think just for consistency, we shouldn't mark non-public APIs. Can you re-run this tool but exclude private classes?
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 we can, but it takes some time... Finding the API is done by the tool, but adding the annotation was done manually.
|
Can one of the admins verify this patch? |
|
@ehsun7b is this still active? if it's not a WIP then you can close it for now |
What changes were proposed in this pull request?
This is the partial fix for this JIRA: https://issues.apache.org/jira/browse/SPARK-15790
Sinceannotation has been added to all classes at packageorg.apache.spark.mlat class level only.Future work is to add
Sinceannotation at method levels for all classes atorg.apache.spark.mlHow was this patch tested?
This fix is documentation stuff. ./dev/run-tests was successful.
Please review http://spark.apache.org/contributing.html before opening a pull request.