Skip to content

Add docs generation script. #192

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

Merged
merged 2 commits into from
Feb 11, 2021
Merged

Conversation

MarkDaoust
Copy link
Member

@MarkDaoust MarkDaoust commented Jan 22, 2021

Here's a basic working version of a docs-generation script for tensorflow.org.

Is this new tools/ directory the right place to put this script?
Does this need a Op generation step like the old tensorflow java api?:

https://www.tensorflow.org/api_docs/java/org/tensorflow/op/core/package-summary
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/tools/docs/build_java_api_docs.py#L64-L75

The script generates this file structure:

https://gist.github.com/MarkDaoust/f3abf18ceef62c8f3d4fbbec0938a3f0

Does anything look out of place to you?

If you want to try it yourself you'll need to install tensorflow's java-doc generator with:

pip install git+https://github.com/tensorflow/docs

@Craigacp
Copy link
Collaborator

It's missing the generated files. They are checked in in tensorflow-core/tensorflow-core-api/src/gen/java and need to be included in the docs generation. As they are checked in we don't need an ops generation step as the source root should always be current.

I mostly got this working but couldn't quite figure out how it was pulling in the css and styling (probably because my build environment was Ubuntu not Google's version of Debian). I'll pull out the version I had tomorrow and see if there is anything else missing.

@MarkDaoust
Copy link
Member Author

They are checked in in tensorflow-core/tensorflow-core-api/src/gen/java

Done. The most recent commit overlays those files onto the others before generating the docs. Seems to work. Both the generated and non-generated ops seem to be there in the new output listing:

https://gist.github.com/MarkDaoust/020b56743a71eb7c6a34cb7e9a0a7e0f

@karllessard
Copy link
Collaborator

Thanks @MarkDaoust , I think /tools is fine for now. @Craigacp , is there anything else you wanted to check before merging the script?

@Craigacp
Copy link
Collaborator

Craigacp commented Feb 3, 2021

I looked through my version of this, and I had to modify the shell script that is called by gen_java to add a bunch of extra jars (JavaCPP and it's dependencies, and protobuf) as otherwise javadoc couldn't resolve the classes properly. Do the AbstractTF* classes look like they were generated right?

@MarkDaoust
Copy link
Member Author

I do get these sorts of warnings when I run it:

Constructing Javadoc information...
/tmp/tmpqinpxe9o/java/org/tensorflow/Output.java:19: error: package org.bytedeco.javacpp does not exist
import org.bytedeco.javacpp.Pointer;
                           ^
/tmp/tmpqinpxe9o/java/org/tensorflow/proto/framework/DataType.java:15: error: package com.google.protobuf does not exist
    implements com.google.protobuf.ProtocolMessageEnum {
                                  ^

I had to modify the shell script that is called by gen_java to add a bunch of extra jars (JavaCPP and it's dependencies, and protobuf)

If you've still got that code handy please send it as a PR to https://github.com/tensorflow/docs where that code lives.

Or just post the code here and I'll take care of it.

@saudet
Copy link
Contributor

saudet commented Feb 4, 2021

I looked through my version of this, and I had to modify the shell script that is called by gen_java to add a bunch of extra jars (JavaCPP and it's dependencies, and protobuf) as otherwise javadoc couldn't resolve the classes properly. Do the AbstractTF* classes look like they were generated right?

No, those are handwritten. Anything that isn't in src/gen isn't generated.

@Craigacp
Copy link
Collaborator

Craigacp commented Feb 4, 2021

I looked through my version of this, and I had to modify the shell script that is called by gen_java to add a bunch of extra jars (JavaCPP and it's dependencies, and protobuf) as otherwise javadoc couldn't resolve the classes properly. Do the AbstractTF* classes look like they were generated right?

No, those are handwritten. Anything that isn't in src/gen isn't generated.

I meant did the javadoc for those classes look like it was generated correctly, which given Mark's response it presumably isn't.

@MarkDaoust I had to add this to the doclet path:

JARS=antlr-complete-3.5.2.jar:doclava-android-6.0.1.jar:guava-29.0-jre.jar:jsilver-android-6.0.1.jar:tagsoup-1.2.1.jar

and these to the classpath:

javacpp-1.5.4.jar:protobuf-java-3.8.0.jar:osgi.annotation-7.0.0.jar

I think the doclet jars might just be an artifact of the fact that I couldn't replicate the environment the docs build in. The classpath ones are because those are referenced from classes.

I dunno why it needs the osgi one, it's referenced by JavaCPP and would crash without it, but I couldn't figure out why it was necessary. @saudet any idea?

@saudet
Copy link
Contributor

saudet commented Feb 4, 2021

@timothyjward added a couple of annotations in the code for OSGi, but I don't know why that would make it crash...

@Craigacp
Copy link
Collaborator

Craigacp commented Feb 4, 2021

It might need to reference them if they appear transitively on subclasses. By crash I mean the javadoc tool error'd out for that class and carried on processing the rest of them.

@timothyjward
Copy link

@timothyjward added a couple of annotations in the code for OSGi, but I don't know why that would make it crash...

Those are class retention annotations used to generate OSGi metadata in the manifest of JavaCPP. They don't exist at runtime, just in the class bytecode. The normal rules for annotation processing are to ignore the annotations if you can't load them or don't understand them, so I can see why there might be a warning (at most) from another tool, but not a crash.

@karllessard
Copy link
Collaborator

I think we are good to merge this now, even if we don't have a clear idea why we need osgi jar to prevent the tool from breaking, as long as it works its fine. Thanks @MarkDaoust !

@karllessard karllessard merged commit 5e7c66a into tensorflow:master Feb 11, 2021
@MarkDaoust
Copy link
Member Author

Thanks guys. I'll try staging this and I'll see if I can get back you with a link where you can preview it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants