[SPARK-19402][DOCS] Support LaTex inline formula correctly and fix warnings in Scala/Java APIs generation #16741

HyukjinKwon · 2017-01-30T13:04:43Z

What changes were proposed in this pull request?

This PR proposes three things as below:

Support LaTex inline-formula, $ ... $ in Scala API documentation
It seems currently,
```
$ ... $
```
are rendered as they are, for example,

It seems mistakenly more backslashes were added.

Fix warnings Scaladoc/Javadoc generation
This PR fixes t two types of warnings as below:

[warn] .../spark/sql/catalyst/src/main/scala/org/apache/spark/sql/Row.scala:335: Could not find any member to link for "UnsupportedOperationException".
[warn]   /**
[warn]   ^

[warn] .../spark/sql/core/src/main/scala/org/apache/spark/sql/internal/VariableSubstitution.scala:24: Variable var undefined in comment for class VariableSubstitution in class VariableSubstitution
[warn]  * `${var}`, `${system:var}` and `${env:var}`.
[warn]      ^

Fix Javadoc8 break

[error] .../spark/mllib/target/java/org/apache/spark/ml/PredictionModel.java:7: error: reference not found
[error]  *                       E.g., {@link VectorUDT} for vector features.
[error]                                       ^
[error] .../spark/mllib/target/java/org/apache/spark/ml/PredictorParams.java:12: error: reference not found
[error]    *                          E.g., {@link VectorUDT} for vector features.
[error]                                            ^
[error] .../spark/mllib/target/java/org/apache/spark/ml/Predictor.java:10: error: reference not found
[error]  *                       E.g., {@link VectorUDT} for vector features.
[error]                                       ^
[error] .../spark/sql/hive/target/java/org/apache/spark/sql/hive/HiveAnalysis.java:5: error: reference not found
[error]  * Note that, this rule must be run after {@link PreprocessTableInsertion}.
[error]                                                  ^

How was this patch tested?

Manually via sbt unidoc and jeykil build.

srowen

Yeah generally I'm in favor of changing anything that causes a build failure in Java 8, or doesn't render correctly. Otherwise if it just generates a warning, I'm kind of neutral on it, but not against it.

srowen · 2017-01-30T13:14:25Z

core/src/main/scala/org/apache/spark/FutureAction.scala

@@ -58,7 +58,7 @@ trait FutureAction[T] extends Future[T] {
   *
   * @param atMost maximum wait time, which may be negative (no waiting is done), Duration.Inf
   *               for unbounded waiting, or a finite positive duration
-   * @throws Exception exception during action execution
+   * @note Throws `Exception` exception during action execution


Ugh, this still generates warnings in unidoc? Does it actually render correctly anyway?

If it renders correctly, I'm inclined to keep it, as long as it doesn't fail the build.
If it didn't render correctly to begin with, OK.

Yes.. It seems fine although it does not have a link.

Scaladoc

Javadoc

Let me revert back those changes.

I see. Yeah it's unfortunate that this (and other) classes aren't found by unidoc. They're JDK classes! I don't see what we could be doing wrong though,

In other cases it's easy enough to change [[ to a back-tick to work around the warning. Here you'd have to mis-use scaladoc a bit to work around it. I think you could leave this, IMHO. The other [[ changes make sense.

HyukjinKwon · 2017-01-30T13:46:01Z

@srowen, ~~let me just turn this PR to a minor without a JIRA for actual errors here.~~

~~Let me consider the warning changes later when I happened to be pretty sure of it, if someone raises this issue again or when any event happens about this .~~

Uh, I just saw #16741 (comment). Then, should I maybe revert only @note changes here?

HyukjinKwon · 2017-01-30T13:55:09Z

Actually, I am still neutral on this and confused which way is more correct. It warns anyhow but IMHO changing [[ to back-ticks which are super strictly a little bit worse because [[ at least have some links in Scaladoc.

HyukjinKwon · 2017-01-30T13:58:04Z

I mean.. it'd be surely better to change it to back-ticks if this breaks but it just warns.. (It is funny that I am saying my proposal should be fixed though). Let me follow your lead please.

HyukjinKwon · 2017-01-30T14:21:37Z

Oh, wait.... it seems case-by-case. Let me leave some before-after image with some explanation soon.

SparkQA · 2017-01-30T15:19:01Z

Test build #72152 has finished for PR 16741 at commit 0880b9b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2017-01-30T15:25:03Z

core/src/main/scala/org/apache/spark/SparkConf.scala

@@ -262,7 +262,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
  /**
   * Get a time parameter as seconds; throws a NoSuchElementException if it's not set. If no
   * suffix is provided then seconds are assumed.
-   * @throws java.util.NoSuchElementException
+   * @throws java.util.NoSuchElementException If the time parameter is not set


Before

Scaladoc

Javadoc

After

Scaladoc

Javadoc

And.. here is my observation so far.

Notation Scaladoc Javadoc Link Error/Warn etc.

`DataFrame` Renders OK Renders OK X X

[[DataFrame]] Renders OK Renders OK Scaladoc only Error in javadoc8 Fixed in #16013

@throws IllegalArgumentException ... Renders OK Renders OK X Warn

@throws java.util.NoSuchElementException ... Renders OK Renders OK X Warn

@throws java.util.NoSuchElementException [NO DESCRIPTION] X Renders OK X Warn Fixed here

HyukjinKwon · 2017-01-30T15:29:48Z

mllib/src/main/scala/org/apache/spark/mllib/optimization/Gradient.scala

@@ -78,7 +78,7 @@ abstract class Gradient extends Serializable {
 *
 * for K classes multiclass classification problem.
 *
- * The model weights $w = (w_1, w_2, ..., w_{K-1})^T$ becomes a matrix which has dimension of
+ * The model weights \(w = (w_1, w_2, ..., w_{K-1})^T\) becomes a matrix which has dimension of


HyukjinKwon · 2017-01-30T15:30:05Z

mllib/src/main/scala/org/apache/spark/mllib/optimization/Gradient.scala

- *       $margins_i = x w_i$.
+ * where $\alpha(i) = 1$ if \(i \ne 0\), and
+ *       $\alpha(i) = 0$ if \(i == 0\),
+ *       \(margins_i = x w_i\).


HyukjinKwon · 2017-01-30T15:30:21Z

mllib/src/main/scala/org/apache/spark/mllib/optimization/Gradient.scala

- * where $\delta_{i, j} = 1$ if $i == j$,
- *       $\delta_{i, j} = 0$ if $i != j$, and
+ * where $\delta_{i, j} = 1$ if \(i == j\),
+ *       $\delta_{i, j} = 0$ if \(i != j\), and


SparkQA · 2017-01-30T17:59:37Z

Test build #72156 has finished for PR 16741 at commit 7e6e521.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-01-30T18:10:11Z

Test build #72157 has finished for PR 16741 at commit 90bf25b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2017-02-01T13:26:37Z

merged to master

HyukjinKwon · 2017-02-01T13:29:09Z

Thank you @srowen!

jkbradley · 2017-02-07T00:19:51Z

Thanks for these many cleanups! It's a shame to lose links. Do you think we should use fully qualified names rather than abandoning the links?

srowen · 2017-02-07T10:35:08Z

I don't think it's a matter of fully-qualified names; it knows what the type is just thinks it can't find it for some reason and raises a warning/error.

HyukjinKwon · 2017-02-07T10:44:51Z

Yes, it can't find. I trieid several possible combinations such as fully qualified name, [[ .. ]] and scala specific notations but I had no luck.

jkbradley · 2017-02-13T21:18:48Z

mllib/src/main/scala/org/apache/spark/ml/util/ReadWrite.scala

@@ -135,13 +135,13 @@ abstract class MLWriter extends BaseReadWrite with Logging {
 }

 /**
- * Trait for classes that provide [[MLWriter]].
+ * Trait for classes that provide `MLWriter`.


@HyukjinKwon Are you sure this is a problem? I just tried generating the docs using Java 8, and the link with [[MLWriter]] worked for both Scala and Java. Maybe it is fine for some cases (like this, where MLWriter is part of the package being compiled) and not for other cases (such as when the class is from a dependency).

Hmm. That's weird. At least, it should warn because I copied and pasted each class names from the messages and went to the file said in the message. Let me double check and be back. Maybe, I made mistakes for few.

@jkbradley, I see. This one seems fine for both.

I think I changed them given def write: MLWriter below...

[warn] .../spark/mllib/src/main/scala/org/apache/spark/ml/util/ReadWrite.scala:143: Could not find any member to link for "MLWriter". [warn] /** [warn] ^

and I think I swept them here as they looked identical. I am okay to revive identified ones back.

My (maybe nitpicking) concern is, we should be able to identify the cases explicitly and what to do in each case which I guess I and many guys failed to do so. Otherwise, I think we should prefer backquotes in such cases because even if reviving links might work for now for some cases, we could easily introduce other breaks again when we change the codes.

…rnings in Scala/Java APIs generation ## What changes were proposed in this pull request? This PR proposes three things as below: - Support LaTex inline-formula, `$ ... $` in Scala API documentation It seems currently, ``` $ ... $ ``` are rendered as they are, for example, <img width="345" alt="2017-01-30 10 01 13" src="https://cloud.githubusercontent.com/assets/6477701/22423960/ab37d54a-e737-11e6-9196-4f6229c0189c.png"> It seems mistakenly more backslashes were added. - Fix warnings Scaladoc/Javadoc generation This PR fixes t two types of warnings as below: ``` [warn] .../spark/sql/catalyst/src/main/scala/org/apache/spark/sql/Row.scala:335: Could not find any member to link for "UnsupportedOperationException". [warn] /** [warn] ^ ``` ``` [warn] .../spark/sql/core/src/main/scala/org/apache/spark/sql/internal/VariableSubstitution.scala:24: Variable var undefined in comment for class VariableSubstitution in class VariableSubstitution [warn] * `${var}`, `${system:var}` and `${env:var}`. [warn] ^ ``` - Fix Javadoc8 break ``` [error] .../spark/mllib/target/java/org/apache/spark/ml/PredictionModel.java:7: error: reference not found [error] * E.g., {link VectorUDT} for vector features. [error] ^ [error] .../spark/mllib/target/java/org/apache/spark/ml/PredictorParams.java:12: error: reference not found [error] * E.g., {link VectorUDT} for vector features. [error] ^ [error] .../spark/mllib/target/java/org/apache/spark/ml/Predictor.java:10: error: reference not found [error] * E.g., {link VectorUDT} for vector features. [error] ^ [error] .../spark/sql/hive/target/java/org/apache/spark/sql/hive/HiveAnalysis.java:5: error: reference not found [error] * Note that, this rule must be run after {link PreprocessTableInsertion}. [error] ^ ``` ## How was this patch tested? Manually via `sbt unidoc` and `jeykil build`. Author: hyukjinkwon <[email protected]> Closes apache#16741 from HyukjinKwon/warn-and-break.

Fix warnings and javadoc8 break

0880b9b

HyukjinKwon changed the title ~~[WIP][SPARK-19402][DOCS] Support LaTex inline annotation correctly and fix warnings in Scala/Java APIs generation~~ [WIP][SPARK-19402][DOCS] Support LaTex inline formula correctly and fix warnings in Scala/Java APIs generation Jan 30, 2017

srowen reviewed Jan 30, 2017

View reviewed changes

HyukjinKwon added 2 commits January 30, 2017 23:54

Revert the changes fron throws to note

341540a

Adds description in throws

da81e0c

HyukjinKwon added 2 commits January 31, 2017 00:19

Revert the other missed one

ea7dd58

typo

7e6e521

HyukjinKwon commented Jan 30, 2017

View reviewed changes

srowen approved these changes Jan 30, 2017

View reviewed changes

HyukjinKwon commented Jan 30, 2017

View reviewed changes

Leave it if I can't fix rather than removing

90bf25b

HyukjinKwon changed the title ~~[WIP][SPARK-19402][DOCS] Support LaTex inline formula correctly and fix warnings in Scala/Java APIs generation~~ [SPARK-19402][DOCS] Support LaTex inline formula correctly and fix warnings in Scala/Java APIs generation Jan 30, 2017

asfgit closed this in f1a1f26 Feb 1, 2017

HyukjinKwon deleted the warn-and-break branch February 1, 2017 13:29

HyukjinKwon mentioned this pull request Feb 2, 2017

[SPARK-19389][ML][PYTHON][DOC] Minor doc fixes for ML Python Params and LinearSVC #16723

Closed

jkbradley reviewed Feb 13, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-19402][DOCS] Support LaTex inline formula correctly and fix warnings in Scala/Java APIs generation #16741

[SPARK-19402][DOCS] Support LaTex inline formula correctly and fix warnings in Scala/Java APIs generation #16741

HyukjinKwon commented Jan 30, 2017 •

edited

Loading

srowen left a comment

srowen Jan 30, 2017

HyukjinKwon Jan 30, 2017

srowen Jan 30, 2017

HyukjinKwon commented Jan 30, 2017 •

edited

Loading

HyukjinKwon commented Jan 30, 2017

HyukjinKwon commented Jan 30, 2017

HyukjinKwon commented Jan 30, 2017

SparkQA commented Jan 30, 2017

HyukjinKwon Jan 30, 2017 •

edited

Loading

HyukjinKwon Jan 30, 2017 •

edited

Loading

HyukjinKwon Jan 30, 2017

HyukjinKwon Jan 30, 2017

HyukjinKwon Jan 30, 2017

SparkQA commented Jan 30, 2017

SparkQA commented Jan 30, 2017

srowen commented Feb 1, 2017

HyukjinKwon commented Feb 1, 2017

jkbradley commented Feb 7, 2017

srowen commented Feb 7, 2017

HyukjinKwon commented Feb 7, 2017

jkbradley Feb 13, 2017

HyukjinKwon Feb 13, 2017 •

edited

Loading

HyukjinKwon Feb 14, 2017 •

edited

Loading

Notation	Scaladoc	Javadoc	Link	Error/Warn	etc.
`DataFrame`	Renders OK	Renders OK	X	X
`[[DataFrame]]`	Renders OK	Renders OK	Scaladoc only	Error in javadoc8	Fixed in #16013
`@throws IllegalArgumentException` ...	Renders OK	Renders OK	X	Warn
`@throws java.util.NoSuchElementException` ...	Renders OK	Renders OK	X	Warn
`@throws java.util.NoSuchElementException` [NO DESCRIPTION]	X	Renders OK	X	Warn	Fixed here

[SPARK-19402][DOCS] Support LaTex inline formula correctly and fix warnings in Scala/Java APIs generation #16741

[SPARK-19402][DOCS] Support LaTex inline formula correctly and fix warnings in Scala/Java APIs generation #16741

Conversation

HyukjinKwon commented Jan 30, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

srowen left a comment

Choose a reason for hiding this comment

srowen Jan 30, 2017

Choose a reason for hiding this comment

HyukjinKwon Jan 30, 2017

Choose a reason for hiding this comment

srowen Jan 30, 2017

Choose a reason for hiding this comment

HyukjinKwon commented Jan 30, 2017 • edited Loading

HyukjinKwon commented Jan 30, 2017

HyukjinKwon commented Jan 30, 2017

HyukjinKwon commented Jan 30, 2017

SparkQA commented Jan 30, 2017

HyukjinKwon Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

HyukjinKwon Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

HyukjinKwon Jan 30, 2017

Choose a reason for hiding this comment

HyukjinKwon Jan 30, 2017

Choose a reason for hiding this comment

HyukjinKwon Jan 30, 2017

Choose a reason for hiding this comment

SparkQA commented Jan 30, 2017

SparkQA commented Jan 30, 2017

srowen commented Feb 1, 2017

HyukjinKwon commented Feb 1, 2017

jkbradley commented Feb 7, 2017

srowen commented Feb 7, 2017

HyukjinKwon commented Feb 7, 2017

jkbradley Feb 13, 2017

Choose a reason for hiding this comment

HyukjinKwon Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

HyukjinKwon Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

HyukjinKwon commented Jan 30, 2017 •

edited

Loading

HyukjinKwon commented Jan 30, 2017 •

edited

Loading

HyukjinKwon Jan 30, 2017 •

edited

Loading

HyukjinKwon Jan 30, 2017 •

edited

Loading

HyukjinKwon Feb 13, 2017 •

edited

Loading

HyukjinKwon Feb 14, 2017 •

edited

Loading