Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable multiprocess docs generation #4302

Merged
merged 1 commit into from Dec 22, 2017
Merged

Enable multiprocess docs generation #4302

merged 1 commit into from Dec 22, 2017

Conversation

udim
Copy link
Member

@udim udim commented Dec 20, 2017

No description provided.

@udim
Enable multiprocess docs generation
f3c498b 
This command is affected: tox -e docs
It is now 40% faster on my workstation: 30s down from 50s.

Fixed exclude_patterns path in Sphinx conf.py.

Updates version of these packages:
Sphinx - bugfix in multiprocess support
google-apitools - to clear warnings about
  oauth2client.contrib.multistore_file
six - version 1.11 no longer incompatible with google-apitools
@udim
Copy link
Member Author

udim commented Dec 20, 2017

Warnings about oauth2client.contrib.multistore_file appeared after upgrading Sphinx.
"tox -e docs" fails on any warnings generated.

Exclude patterns were fixed because the multiprocess runs had this warning:
apache_beam.rst: WARNING: document isn't included in any toctree

@udim
Copy link
Member Author

udim commented Dec 20, 2017

R: @aaltay

aaltay
aaltay reviewed Dec 20, 2017
View reviewed changes
Copy link
Member

@aaltay aaltay left a comment

Choose a reason for hiding this comment

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

Thank you.

Pinged a few people for the apitools change, in case there is a potential issue that they are aware of.

sdks/python/setup.py
@@ -121,7 +119,7 @@ def get_version():
]

GCP_REQUIREMENTS = [
'google-apitools>=0.5.10,<=0.5.11',
Copy link
Member

Choose a reason for hiding this comment

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

Have you tested this by running a job on Dataflow?

This is a little scary because the latest version of apitools on github is 0.5.14.

@craigcitro @charlesccychen @chamikaramj (FYI, in case anybody is aware of a known incompatibility.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Python SDK PostCommit Tests have successfully completed, which use TestDataflowRunner.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome! Thank you.

sdks/python/generate_pydoc.sh
@@ -167,7 +167,7 @@ EOF

# Build the documentation using sphinx
# Reference: http://www.sphinx-doc.org/en/stable/man/sphinx-build.html
python $(type -p sphinx-build) -v -a -E -q target/docs/source \
python $(type -p sphinx-build) -v -a -E -j 8 -q target/docs/source \
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 use the long forms for these arguments if possible.

Copy link
Member Author

@udim udim Dec 20, 2017 •
edited

Choose a reason for hiding this comment

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

AFAIK there isn't a long form for this.
http://www.sphinx-doc.org/en/stable/invocation.html#invocation-of-sphinx-build

Copy link
Member

Choose a reason for hiding this comment

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

:/ thank you for checking!

@craigcitro
Copy link
Contributor

/cc @znewman01 for the apitools question

@aaltay
Copy link
Member

aaltay commented Dec 20, 2017

@znewman01 Seems like there is a new github repo for apitools: https://github.com/google/apitools, pypi page (https://pypi.python.org/pypi/google-apitools) is still pointing to the old page.

@udim
Copy link
Member Author

udim commented Dec 21, 2017

jenkins: run python postcommit

@udim
Copy link
Member Author

udim commented Dec 21, 2017

run python postcommit

@znewman01
Copy link

To answer a couple of questions:

  • Yes, google/apitools is the new and up-to-date repo for apitools. Will get that page updated.

  • There shouldn't be any breaking changes; I checked git log v0.5.11..v0.5.20 and only see:

    • some file upload logic changes (v0.5.19 might cause you to run out of memory uploading a file in a chunk size greater than the amount of free memory you have, but with reasonable chunk sizes should be a non-issue
    • minor auth changes (mostly bugfixes and upgrading oauth library versions
    • don't fail on copying unknown Enum values
    • new features (compression)/sugar (mapping between Python dicts and Message maps)
    • testing/error changes (backwards compat)
    • fix some encoding issues for bytes fields in messages
    • unicode support

    These all seem backwards-compatible.

+CC @kevinli7 FYI

@aaltay
Copy link
Member

aaltay commented Dec 21, 2017

@znewman01 Thank you! I appreciate the detailed information.

Could you clarify two points:

  1. We pass a custom http client to BaseApiClient, using http argument. Is there a change related to that?
  2. Related to oauth2client. We upper-bounded Beam client to <4.0.0 mainly because of the previous apitools incompatibility. (I believe it was error like ImportError: cannot import name locked_file). Could we remove this limitation now?

@znewman01
Copy link

  1. I wouldn't expect any changes in that functionality.

  2. 0.5.18 is the apitools version that first has full outh2client 4.0+ support (via 63e97c16 and cbf9c60f). So with your change as written (e.g. allowing 0.5.10..0.5.20) I'd say no. Once your lower bound is 0.5.18, should be fine.

@aaltay
Copy link
Member

aaltay commented Dec 22, 2017

Thank you @znewman01.

We should consider upgrading our apitools dependency post this change (https://issues.apache.org/jira/browse/BEAM-3391)

@aaltay aaltay merged commit 28c92e4 into apache:master Dec 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaltay aaltay aaltay left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@udim @craigcitro @aaltay @znewman01