[ML-5700] Update release script to build and push docs. #176

MrBago · 2018-11-30T02:34:11Z

This PR adds doc building to the release script. The script will checking the release tag and build the docs in docker. It will then push the docs branch, eg "gh-pages-1.4.0" upstream & replace the "gh-pages" branch.

I opted for replacing the gh-pages branch instead of creating a PR because this type of PR will inherently have conflicts.

mengxr · 2018-11-30T03:46:01Z

dev/build-docs.sh

 # build the API docs
-docker run --rm -v "$(pwd):/mnt/sparkdl" databricks/sparkdl-docs dev/build-docs-in-docker.sh
+docker run --rm -v "$(pwd):/mnt/sparkdl" databricks/sparkdl-docs \
+    bash -i -c dev/build-docs-in-docker.sh


Shall we make a separate PR for this fix?

I can if you think we should, it's tiny so I was just folding it in.

It is tiny but it is a separate fix and it doesn't need to wait for review of the release script to get in. Let's keep it with this PR but create separate PRs in the future to keep each minimal.

mengxr

We need to modify "docs/_config.yml" to update release version there or change "docs/_plugins/copy_api_dirs.rb" to get the version automatically.
It would be nice to allow doc build/release independently. One package release must be paired with doc release. However, we might need to do small doc fixes after release.

Both could be done in follow-up PRs.

mengxr · 2018-11-30T03:46:41Z

dev/release.py

-WORKING_BRANCH = "PREP_RELEASE_%s"
 DATABRICKS_REMOTE = "git@github.com:databricks/spark-deep-learning.git"
-PUBLISH_MODES = {"local": "publishLocal", "m2": "publishM2", "spark-package": "spDist"}
+PUBLISH_MODES = {"local": "publishLocal", "m2": "publishM2", "spark-package-local": "spDist",


"spark-package-local", "spark-package-publish" not used

mengxr · 2018-11-30T03:47:40Z

dev/release.py

 DATABRICKS_REMOTE = "git@github.com:databricks/spark-deep-learning.git"
-PUBLISH_MODES = {"local": "publishLocal", "m2": "publishM2", "spark-package": "spDist"}
+PUBLISH_MODES = {"local": "publishLocal", "m2": "publishM2", "spark-package-local": "spDist",
+                 "spark-package-publish": "sbPublish"}


spPublish? If the feature is not implemented, please remove it and keep this PR minimal.

Sorry I don't quite follow, I think we need spPublish publishing to spark-packages, I can drop spDist.

It was "sbPublish" instead of "spPublish". My ask is mainly why it shows up in a PR that is titled "doc build"?

mengxr · 2018-11-30T03:55:22Z

dev/release.py

-    if working_branch in check_output(["git", "branch"]):
-        prominentPrint(
-            "Working branch %s already exists, please delete it and try again." % working_branch)
+    gh_pages_branch = GH_PAGES_BRANCH % release_version


rename it to "gh_pages_release_branch" or "gh_pages_working_branch".

This is just a working branch. I would append a timestamp at the end to avoid conflict if I need to rerun the script. (Same for working_branch.

mengxr · 2018-11-30T03:55:46Z

dev/release.py

-            "Working branch %s already exists, please delete it and try again." % working_branch)
+    gh_pages_branch = GH_PAGES_BRANCH % release_version
+    local_branches = check_output(["git", "branch"])
+    target_branches = [gh_pages_branch, working_branch, "gh-pages"]


It is common to have "gh-pages" as a local branch. The script doesn't depend on this local branch.

mengxr · 2018-11-30T04:02:54Z

dev/release.py

+    gh_pages_branch = GH_PAGES_BRANCH % release_version
+    local_branches = check_output(["git", "branch"])
+    target_branches = [gh_pages_branch, working_branch, "gh-pages"]
+    conflict_branches = list(filter(lambda a: a in local_branches, target_branches))


This check is flaky. I have a local branch called "gh-pages-bak".

mengxr · 2018-11-30T04:03:29Z

dev/release.py

-        prominentPrint(
-            "Working branch %s already exists, please delete it and try again." % working_branch)
+    gh_pages_branch = GH_PAGES_BRANCH % release_version
+    local_branches = check_output(["git", "branch"])


For all "check_output`, we need ".decode()" in py3. If the script only works for py2, put an assertion at the top.

I updated the script, it works with python 2 & 3 now.

mengxr · 2018-11-30T04:18:16Z

dev/release.py

+    msg = "Would you like to push docs branch to remote, %s, and update gh-pages branch? (y/n)"
+    msg %= git_remote
+    if verify(msg, interactive):
+        check_call(["git", "push", git_remote, gh_pages_branch])


Do we need to push the working branch? The next command force update the doc branch already. Is it for a quick revert back to this version in the future?

Ya this is for quick revert in the future, but actually I think it should be a tag not a branch. I've made that change.

The problem with using a tag is that it shows up in the "releases" section of the github ui. I'll work on this tomorrow.

mengxr · 2018-11-30T18:35:12Z

dev/release.py

-WORKING_BRANCH = "PREP_RELEASE_%s"
 DATABRICKS_REMOTE = "git@github.com:databricks/spark-deep-learning.git"
-PUBLISH_MODES = {"local": "publishLocal", "m2": "publishM2", "spark-package": "spDist"}
+PUBLISH_MODES = {"local": "publishLocal", "m2": "publishM2", "spark-package-publish": "spPublish"}


I still don't know why the changes are relevant to "build and push docs". If they are necessary to include, it is worth an inline GitHub comment here. If they are irrelevant, we should create a separate PR.

Keeping PRs minimal is to help both PR review and merge. For example, I spent ~10 mins yesterday trying to understand how it is relevant to doc build but got no clue.

This isn't related to docs, I should have commented sooner.

I realized while working on docs that we'll need this publish method available to use this for an actual release. I can split this into another PR if you think we should.

mengxr · 2018-11-30T18:45:52Z

dev/release.py

-    if working_branch in check_output(["git", "branch"]):
-        prominentPrint(
-            "Working branch %s already exists, please delete it and try again." % working_branch)
+    working_branch = WORKING_BRANCH % (release_version, time.hour, time.minute, time.second)


It is missing date. It might be easier convert the timestamp into a single string first and include it in the template here.

We need to use a custom format for the datetime because the default has characters that cannot be used in a git branch name, I'll update the custom format to include the date.

mengxr · 2018-11-30T18:46:00Z

dev/release.py

-        prominentPrint(
-            "Working branch %s already exists, please delete it and try again." % working_branch)
+    working_branch = WORKING_BRANCH % (release_version, time.hour, time.minute, time.second)
+    gh_pages_branch = WORKING_DOCS_BRANCH % (release_version, time.hour, time.minute, time.second)


MrBago · 2018-12-01T00:44:40Z

@mengxr I think this is ready for another review, I'm also working on a PR to update the _config.yml file as part of the release. I tried changing docs/_plugins/copy_api_dirs.rb to use version.sbt instead of the config file, but it seems that jekyll reads the config file too, so that only updated some of the version references but not all.

mengxr · 2018-12-01T06:03:25Z

The doc release script looks good. I'm merging this PR in. However, there might be an issue with doc build. I cannot find the public APIs in the generated Python API html page. Could you see if this happens on your laptop?

mengxr · 2018-12-01T06:10:04Z

This is the error I hit:

/mnt/sparkdl/python/docs/index.rst:15: WARNING: toctree contains reference to nonexisting document u' sparkdl.graph'
/mnt/sparkdl/python/docs/index.rst:15: WARNING: toctree contains reference to nonexisting document u' sparkdl.image'
/mnt/sparkdl/python/docs/index.rst:15: WARNING: toctree contains reference to nonexisting document u' sparkdl.transformers'
/mnt/sparkdl/python/docs/index.rst:15: WARNING: toctree contains reference to nonexisting document u' sparkdl.udf'
/mnt/sparkdl/python/docs/index.rst:15: WARNING: toctree contains reference to nonexisting document u' sparkdl.utils'
WARNING: autodoc: failed to import module u'sparkdl'; the following exception was raised:
No module named tensorframes
WARNING: autodoc: failed to import module u'graph.input' from module u'sparkdl'; the following exception was raised:
No module named tensorframes
WARNING: autodoc: failed to import module u'graph.utils' from module u'sparkdl'; the following exception was raised:
No module named tensorframes
WARNING: autodoc: failed to import module u'image.imageIO' from module u'sparkdl'; the following exception was raised:
No module named tensorframes
WARNING: autodoc: failed to import module u'transformers.utils' from module u'sparkdl'; the following exception was raised:
No module named tensorframes
WARNING: autodoc: failed to import module u'udf.keras_image_model' from module u'sparkdl'; the following exception was raised:
No module named tensorframes
WARNING: autodoc: failed to import module u'utils.keras_model' from module u'sparkdl'; the following exception was raised:
No module named tensorframes

Update release script to build and push docs.

09d83dc

mengxr reviewed Nov 30, 2018

View reviewed changes

mengxr requested changes Nov 30, 2018

View reviewed changes

Some chages based on PR.

86f8a16

mengxr requested changes Nov 30, 2018

View reviewed changes

Fix datetime and push branch instead of tag.

8e724f6

mengxr approved these changes Dec 1, 2018

View reviewed changes

mengxr merged commit c176516 into databricks:master Dec 1, 2018

[ML-5700] Update release script to build and push docs. #176

[ML-5700] Update release script to build and push docs. #176

Uh oh!

Conversation

MrBago commented Nov 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mengxr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrBago commented Dec 1, 2018

Uh oh!

mengxr commented Dec 1, 2018

Uh oh!

mengxr commented Dec 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants