Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[MXNET-319] Javadoc fix #11239

Merged
merged 19 commits into from
Jul 3, 2018
Merged

[MXNET-319] Javadoc fix #11239

merged 19 commits into from
Jul 3, 2018

Conversation

lanking520
Copy link
Member

@lanking520 lanking520 commented Jun 12, 2018

Description

This is the second proposed solution to this: #11123
Add Java doc support.
@nswamy @yzhliu @andrewfayres
Be aware, I have imported a third party library to generate the Scala doc. Please check the pom file carefully.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@lanking520 lanking520 requested a review from yzhliu as a code owner June 12, 2018 01:05
@lanking520 lanking520 mentioned this pull request Jun 12, 2018
5 tasks
@@ -67,7 +67,7 @@
<linkerMiddleOption>-Wl,-x</linkerMiddleOption>
<linkerMiddleOption>${lddeps}</linkerMiddleOption>
<linkerMiddleOption>-force_load ../../../lib/libmxnet.a</linkerMiddleOption>
<linkerMiddleOption>-force_load ../../../3rdparty/tvm/nnvm/lib/libnnvm.a</linkerMiddleOption>
<linkerMiddleOption>-force_load ../../../3rdparty/nnvm/lib/libnnvm.a</linkerMiddleOption>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, it just live there for a long time. And do you know who change that line of code. I pulled from master and see that addition

@lanking520 lanking520 requested a review from szha as a code owner June 13, 2018 18:14
@lanking520 lanking520 force-pushed the javadoc-fix3 branch 2 times, most recently from e9ab9d7 to 4d1954f Compare June 14, 2018 18:04
@@ -823,7 +823,7 @@ class Symbol private(private[mxnet] val handle: SymbolHandle) extends WarnIfNotD
}

@AddSymbolFunctions(false)
object Symbol {
object Symbol extends SymbolBase {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add comments to all class/object/method definitions.

@@ -37,6 +37,8 @@ private[mxnet] object APIDocGenerator{
val FILE_PATH = args(0)
absClassGen(FILE_PATH, true)
absClassGen(FILE_PATH, false)
oldAPIClassGen(FILE_PATH, true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oldAPIClassGen->NonTypeSafeClassGen

@@ -61,8 +63,35 @@ private[mxnet] object APIDocGenerator{
pw.close()
}

def oldAPIClassGen(FILE_PATH : String, isSymbol : Boolean) : Unit = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nonTypeSafeAPIClassGen

// Generate ScalaDoc type
def generateAPIDocFromBackend(func : absClassFunction) : String = {
def generateAPIDocFromBackend(func : absClassFunction, withParam : Boolean = true) : String = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please make sure that the documentation matches what we have today, i think we follow Java doc style and not Scala doc style..make sure its consistent as well. I can't tell from the text

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<artifactId>scalastyle-maven-plugin</artifactId>
</plugin>
<plugin>
<groupId>net.alchim31.maven</groupId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we using the plugin from this group? we use it from this group org.scala-tools. Please check pom file on scala-package.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently I cannot find a better approach to generate a doc jar rather than this plugin. As we discovered before, the Javadoc plugin that Scala Package originally have did not compile the Scala code and generate jars.

@lanking520
Copy link
Member Author

Currently, I created a file called FILEHASH lives in the core used to check if there is a change on generated base files. Add a require as well to alert developers. Please check the Hash file and leave your thinkings. I used MD5 in here

@lanking520
Copy link
Member Author

Currently, I cannot make it pass the GPU test even if the MD5 is guaranteed unchanged. I cannot reproduce the problem even if I open a GPU instance and build all over again. In this case, I decided to remove GPU test from the the MD5 checking. It seemed the code generation result are someway different between CIs. Need to figure out in the future

@@ -0,0 +1,4 @@
309G_O39xojNEt3DXVWNDQ
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to be checked in? what files does this hash represent? how are they going to be updated?

Copy link
Member Author

@lanking520 lanking520 Jun 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. The hashfile contains the signature files that on Scala package not uploaded here. It follows the following format:

abstract class SymbolAPIBase {
   /**
    * Comments
    */
   def Activation(...Args...) : Symbol
}

I used md5 hash in here:

  def MD5Generator(input : String) : String = {
    val md = MessageDigest.getInstance("MD5")
    md.update(input.getBytes("UTF-8"))
    val digest = md.digest()
    org.apache.commons.codec.binary.Base64.encodeBase64URLSafeString(digest)
  }

It basically take the Generated string into hash and store in this file. It should be the same across the different platform. The reason to do is is to avoid operator changes in Symbol and NDArray. One example, if one contributor changed the operators in NDArray and Symbol, he/she should comment the require line explained before in Scala package and update this file and also tag Scala Contributors to let them know there is a change in operator so we can determine if this will influence the user experience on Scala.

@lanking520 lanking520 force-pushed the javadoc-fix3 branch 4 times, most recently from 167dacf to 980109b Compare June 23, 2018 22:43
@lanking520 lanking520 force-pushed the javadoc-fix3 branch 2 times, most recently from 9d4f790 to 7d74310 Compare June 28, 2018 21:26
Copy link
Member

@nswamy nswamy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lanking520. Great effort, one last request to change.

def nonTypeSafeClassGen(FILE_PATH : String, isSymbol : Boolean) : String = {
// scalastyle:off
val absClassFunctions = getSymbolNDArrayMethods(isSymbol)
val absFuncs = absClassFunctions.filterNot(_.name.startsWith("_")).map(absClassFunction => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this filter to the getSymbolNDArrayMethods, so we have one place to change in when start adding support for _sparse_ etc.,

Copy link
Member

@nswamy nswamy Jun 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also do this for the NDArrayMacro/SymbolMacro in a separate PR

Copy link
Member

@nswamy nswamy Jul 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lanking520 please change the filter location, so i can merge. I meant in APIDocGenerator

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nswamy I have done that, please double check in the code

@nswamy nswamy merged commit 815eb60 into apache:master Jul 3, 2018
@lanking520 lanking520 deleted the javadoc-fix3 branch July 3, 2018 22:36
XinYao1994 pushed a commit to XinYao1994/incubator-mxnet that referenced this pull request Aug 29, 2018
* Create Interface for Symbol and NDArray APIs, enable JavaDoc jar building for Scala Package.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants