Skip to content

Conversation

@armando-fandango
Copy link
Contributor

@armando-fandango armando-fandango commented Apr 4, 2019

  • Adding initial version of Document Generator
  • modified from tensorflow/tools/docs
  • The docs can be created and pushed to /docs
  • Once this pull request is accepted, we can add the command to "generate and push the docs" to build pipeline.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@armando-fandango
Copy link
Contributor Author

Reopening to fix CLA

@ewilderj ewilderj added cla: yes and removed cla: no labels Apr 5, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@ewilderj
Copy link
Contributor

ewilderj commented Apr 5, 2019

Overrode the CLA status, submitter used wrong email address in error.

Copy link
Member

@seanpmorgan seanpmorgan left a comment

Choose a reason for hiding this comment

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

Thanks @armando-fandango . Generally looks good but some minor things we need to do in order to integrate it with our pipeline


Requires a local installation of:
Docs Generator - pip install git+https://github.com/tensorflow/docs
tensorflow_addons
Copy link
Member

@seanpmorgan seanpmorgan Apr 8, 2019

Choose a reason for hiding this comment

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

So the reason there needed to be a local install of tensorflow (in the tensorflow/docs/generate2.py) was because the script used an installed package to generate docs (and didn't require a heavy TF rebuild).

Thats a bit hacky imo and we should integrate doc generation with our bazel build / testing

Copy link
Member

Choose a reason for hiding this comment

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

something like configure.sh? Sean, note that it's tensorflow-doc, rather than tensorflow package.

Copy link
Member

@seanpmorgan seanpmorgan Apr 11, 2019

Choose a reason for hiding this comment

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

Yeah we could automate the install/generation with a shell script, but we should be able to build the docs using bazel without the need to pip install the package first (which would require a nightly build that we do not have yet).

See the "note" in this section for the reason why they used a pip install instead of just using the source code: https://www.tensorflow.org/community/contribute/documentation#python

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seanpmorgan @facaiy : Two questions here:

  1. Should I still rely on doc s generator in tensorflow_docs .. or that is not an option ?
  2. Should the docs be generated from installed version of addons or from source code version of addons ? if Source code version then I assume it would always be from master branch, Or do we want to keep docs for multiple versions like in TF API docs you can select previous version and see docs generated for previous versions too.

Copy link
Member

Choose a reason for hiding this comment

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

To import docs into tensorflow.org, each "subsite" has a config file that specifies the branch for narrative docs and the branch for API docs.
Typically, narrative docs are set to 'master' since it's easier if browsing the GitHub repo looks close to the site (minus changes from last publish date).
For API docs, we typically publish from a release branch (though it could be a tag). That way you can point to older versions of generated docs in GitHub and the Markdown is still readable. The downside is that you need to bump the (internal) config file whenever you make a release and want to publish it. But we can come up with process for that, or just ping me.

"""A tool to generate api_docs for TensorFlow Addons.

```
Run from root folder of source code
Copy link
Member

Choose a reason for hiding this comment

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

So I believe this will fail if we run as a python file from root. Throws a .so file not found error because they are not built in the repo. We'll need to incorporate this into the bazel BUILD so that docs are generated after the files are compiled.

Copy link
Member

@facaiy facaiy Apr 11, 2019

Choose a reason for hiding this comment

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

There seems at least two solutions:

  1. build and install tf_addons in local machine, and then run generate.py manually.
  2. like build_pip_pkg.sh, we create a shell script which runs generate.py, and write a sh_binary target in bazel BUILD.
    pros: automatically compile c++ codes
    cons: cannot pass argument, say --output_dir, --code_url_prefix

Copy link
Member

@facaiy facaiy Apr 11, 2019

Choose a reason for hiding this comment

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

@MarkDaoust Hi, Mark, can you introduce how tensorflow team use generate.py file?

Copy link
Member

Choose a reason for hiding this comment

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

Yes a best practice from docs team would be best.

The third option would be:
3. Run the doc generator from a bazel build command. Which is what was insinuated as possible from the note linked above (i think)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#46

I would suggest to run it from Bazel build and then check in the docs into git repo from there. One of the points in discussion in #46 was that once we push docs in /docs folder in our git repo, then tensorflow core team would be able to pull them into community docs from there.

Copy link
Member

Choose a reason for hiding this comment

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

  1. build and install tf_addons in local machine, and then run generate.py manually.
  2. Run the doc generator from a bazel build command.

I think either of these work, the generator just wants access to the module as a user would see it.
How you set it up is only a question of whether you want to use bazel to generate your docs or not.
If you don't do anything too out of the ordinary, you may be able to do (1) even if you have the bazel BUILD files setup for (3).

Bazel build and then check in the docs into git repo from there. ... then tensorflow core team would be able to pull them into community docs from there.

Yes, that would be the least-friction path for us. It also gives your users somewhere to read the docs independent of what happens with this tensorflow.org integration.

from tensorflow.python.util import tf_inspect

# Use tensorflow's `tf_inspect`, which is aware of `tf_decorator`.
parser.tf_inspect = tf_inspect
Copy link
Member

Choose a reason for hiding this comment

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

Can we confirm if this is required? I'm not aware of us using tf_decorator, but it can probably be empirically checked. If we don't need to use this that's great because it's leveraging a private API

Copy link
Member

@facaiy facaiy Apr 11, 2019

Choose a reason for hiding this comment

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

Sean, it seems that tensorflow_docs cannot recognize those imported modules/function, such as keras_utils, absolute_import etc. Not sure whether that's one reason to use tf_export in tensorflow. cc @lamberta

Screen Shot 2019-04-11 at 4 48 48 PM

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
Contributor Author

Choose a reason for hiding this comment

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

It will be good to find out a way to link them back to the documentation in TF core.

Copy link
Member

Choose a reason for hiding this comment

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

Can we confirm if this is required? I'm not aware of us using tf_decorator, but it can probably be empirically checked.

Correct, this line only does anything for you if you're using tf_decorator.

it seems that tensorflow_docs cannot recognize those imported modules/function, such as keras_utils, absolute_import etc. Not sure whether that's one reason to use tf_export in tensorflow.

I'm not sure I understand the question here what is the problem, what are we trying to acomplish?.
I should block the generator from ever mentioning "absolute_import", "division", "print_function".

Note that one of the functions of the base_dir argument is to allow it to filter out objects which are imported by your package, but not part of your package. No docs are generated for objects defined in files outside of base_dir.



def build_docs(output_dir, code_url_prefix, search_hints=True):
"""Build api docs for tensorflow v2.
Copy link
Member

Choose a reason for hiding this comment

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

for tensorflow_addons

"https://github.com/tensorflow/addons/tree/master/tensorflow_addons")

doc_generator = generate_lib.DocGenerator(
root_title="TensorFlow Addons 2.0 Preview",
Copy link
Member

Choose a reason for hiding this comment

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

Just TensorFlow Addons -- no 2.0 preview

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, yes will fix it.

# See the License for the specific language governing permissions and
# limitations under the License.
# ==============================================================================
"""Tests for addons.tools.docs.generate2."""
Copy link
Member

@seanpmorgan seanpmorgan Apr 8, 2019

Choose a reason for hiding this comment

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

We'll need to integrate this with bazel so this test actually gets ran

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just moved it as it is and fixed it, but if we are generating docs through CI pipeline then we can get rid of this test file completely. What do you think ?

import shutil

import tensorflow as tf
from tensorflow.python.platform import googletest
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is just used for a temp dir? Could we replace this with something that doesn't use a private API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as per above comment, I believe we can eliminate this test file completely.

code_url_prefixes = (
code_url_prefix,
# External packages source repositories
"https://github.com/tensorflow/addons/tree/master/tensorflow_addons")
Copy link
Member

Choose a reason for hiding this comment

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

I think we should remove the line, and run the script like: python tools/docs/generate2.py --output_dir=do cs/ --code_url_prefix=https://github.com/tensorflow/addons/tree/master/tensorflow_addons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

code URL prefix would be fixed. What would be the benefit of moving it as an arg?

Copy link
Member

Choose a reason for hiding this comment

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

Unless you're trying to document multiple installed pacakages you likely want a single string for both base_dir and code_url_prefix.

I think what you want is:

base_dir = path.dirname(tfa.__file__)
code_url_prefix =  "https://github.com/tensorflow/addons/tree/master/tensorflow_addons"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Mark, Yan, Sean, will update with these changes in this week.

Copy link
Member

Choose a reason for hiding this comment

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

Unless you're trying to document multiple installed pacakages you likely want a single string for both base_dir and code_url_prefix.

I think what you want is:

base_dir = path.dirname(tfa.__file__)
code_url_prefix =  "https://github.com/tensorflow/addons/tree/master/tensorflow_addons"

For 0.1 version, we should redirect link to r0.1 branch, instead of master branch. That's why I think we need code_url_prefix argument. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@facaiy As I also asked above, do we want to make the doc generator such that it generates docs for all tagged versions in the github repo, like in TF API you can view docs for all previous versions too, or do we want to just keep the docs for latest master branch ?


Requires a local installation of:
Docs Generator - pip install git+https://github.com/tensorflow/docs
tensorflow_addons
Copy link
Member

Choose a reason for hiding this comment

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

something like configure.sh? Sean, note that it's tensorflow-doc, rather than tensorflow package.

search_hints: Bool. Include meta-data search hints at the top of file.
"""
try:
doc_controls.do_not_generate_docs(tfa.tools)
Copy link
Member

Choose a reason for hiding this comment

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

tfa.utils, rather than tfa.tools?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

utils we want to generate documentation of.. right? Just not tools package which only has install and doc generator stuff.

And yes we would need to install tensorflow docs from configure.sh as generate2.py relies on code from that repo.

from tensorflow.python.util import tf_inspect

# Use tensorflow's `tf_inspect`, which is aware of `tf_decorator`.
parser.tf_inspect = tf_inspect
Copy link
Member

@facaiy facaiy Apr 11, 2019

Choose a reason for hiding this comment

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

Sean, it seems that tensorflow_docs cannot recognize those imported modules/function, such as keras_utils, absolute_import etc. Not sure whether that's one reason to use tf_export in tensorflow. cc @lamberta

Screen Shot 2019-04-11 at 4 48 48 PM

Copy link
Member

@MarkDaoust MarkDaoust left a comment

Choose a reason for hiding this comment

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

LMK if you have any questions, need clarifications, or fixes to the docs tools.

"""A tool to generate api_docs for TensorFlow Addons.

```
Run from root folder of source code
Copy link
Member

Choose a reason for hiding this comment

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

  1. build and install tf_addons in local machine, and then run generate.py manually.
  2. Run the doc generator from a bazel build command.

I think either of these work, the generator just wants access to the module as a user would see it.
How you set it up is only a question of whether you want to use bazel to generate your docs or not.
If you don't do anything too out of the ordinary, you may be able to do (1) even if you have the bazel BUILD files setup for (3).

Bazel build and then check in the docs into git repo from there. ... then tensorflow core team would be able to pull them into community docs from there.

Yes, that would be the least-friction path for us. It also gives your users somewhere to read the docs independent of what happens with this tensorflow.org integration.

parser.tf_inspect = tf_inspect

# So patch `tfa.__all__` to list everything.
tfa.__all__ = [item_name for item_name, value in tf_inspect.getmembers(tfa)]
Copy link
Member

Choose a reason for hiding this comment

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

This line is likely not necessary.

The doc generator respects a module's __all__ but will document everything if all is not set. I only added this for tensorflow/tensorflow because there was a miss-match between what was in all and what we wanted to document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks.. will remove it.

from tensorflow.python.util import tf_inspect

# Use tensorflow's `tf_inspect`, which is aware of `tf_decorator`.
parser.tf_inspect = tf_inspect
Copy link
Member

Choose a reason for hiding this comment

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

Can we confirm if this is required? I'm not aware of us using tf_decorator, but it can probably be empirically checked.

Correct, this line only does anything for you if you're using tf_decorator.

it seems that tensorflow_docs cannot recognize those imported modules/function, such as keras_utils, absolute_import etc. Not sure whether that's one reason to use tf_export in tensorflow.

I'm not sure I understand the question here what is the problem, what are we trying to acomplish?.
I should block the generator from ever mentioning "absolute_import", "division", "print_function".

Note that one of the functions of the base_dir argument is to allow it to filter out objects which are imported by your package, but not part of your package. No docs are generated for objects defined in files outside of base_dir.


FLAGS = flags.FLAGS

flags.DEFINE_string(
Copy link
Member

Choose a reason for hiding this comment

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

I think it's safe to just hard code the code_url_prefix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I agree, will do that.

code_url_prefixes = (
code_url_prefix,
# External packages source repositories
"https://github.com/tensorflow/addons/tree/master/tensorflow_addons")
Copy link
Member

Choose a reason for hiding this comment

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

Unless you're trying to document multiple installed pacakages you likely want a single string for both base_dir and code_url_prefix.

I think what you want is:

base_dir = path.dirname(tfa.__file__)
code_url_prefix =  "https://github.com/tensorflow/addons/tree/master/tensorflow_addons"

@facaiy facaiy self-assigned this Apr 15, 2019
@facaiy
Copy link
Member

facaiy commented Apr 16, 2019

@seanpmorgan Sean, after discussion with @armando-fandango : 1) We plan to address current comments and then make sure doc generator works nicely from command line; 2) and then someone else, who're familiar enough with bazel, can take over from there in a separate PR for bazel integration.

What do you think?

@seanpmorgan
Copy link
Member

@seanpmorgan Sean, after discussion with @armando-fandango : 1) We plan to address current comments and then make sure doc generator works nicely from command line; 2) and then someone else, who're familiar enough with bazel, can take over from there in a separate PR for bazel integration.

What do you think?

Sounds great. Thanks everyone for the reviews and iteration

@lamberta
Copy link
Member

FYI, @MarkDaoust added an example build_docs.py script to use the API generator: https://github.com/tensorflow/docs/blob/master/tools/templates/build_docs.py

Teams typically keep this in their tools/ directory (or wherever you want), and then it outputs to docs/api_docs/python/ (for example, see TF Datasets). This is the path we need to add to out importer config to bring it into the site.

@armando-fandango armando-fandango requested a review from a team as a code owner April 21, 2019 22:41
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@armando-fandango
Copy link
Contributor Author

closing this and creating a fresh pull request as the API generator is changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants