[AIRFLOW-2857] Fix broken RTD env #3703

tedmiston · 2018-08-06T02:50:29Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-2857
- In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.

Description

Here are some details about my PR, including screenshots of any UI changes:

Per the mailing list thread "Readthedocs is Broken" on Aug 5, 2018, the Read the Docs build process is currently broken per the recent addition of #3660.

The RTD build environment provides a READTHEDOCS env var boolean which it sets to true. I have used that to toggle on SLUGIFY_USES_TEXT_UNIDECODE only when that env var is supplied to restrict it to that environment.

Related change: The mock module is used in conf.py is supplied to Sphinx to build the docs, but it was missing from the doc install extra in setup.py, so I've added that.

Here's how I tested this:

pip install -e .[doc]  # (fails)

READTHEDOCS=True pip install -e .[doc]  # (succeeds)

cd docs/
./build.sh
./start_doc_server.sh

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

A successful build on https://readthedocs.org/projects/airflow/builds/ will confirm that this is working in the Read the Docs environment.

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

Passes git diff upstream/master -u -- "*.py" | flake8 --diff

tedmiston · 2018-08-06T02:52:52Z

/cc @kaxil

codecov-io · 2018-08-06T03:27:56Z

Codecov Report

Merging #3703 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3703   +/-   ##
=======================================
  Coverage   77.56%   77.56%           
=======================================
  Files         204      204           
  Lines       15766    15766           
=======================================
  Hits        12229    12229           
  Misses       3537     3537

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa7b25b...7007e95. Read the comment docs.

ashb · 2018-08-06T07:46:27Z

setup.py

Could you add a comment saying why this is needed please?

ashb · 2018-08-06T07:47:48Z

setup.py

?? This is needed for docs? I would have though mock is just used in tests/, and i’m Surprised it’s needed here?

Good point. @tedmiston Any idea?

I did some digging and looks like we use it here:
https://github.com/apache/incubator-airflow/blob/acca61c602e341da06ebee2eca3a26f4e7400238/docs/conf.py#L16

This is used for mocking import of various modules:
https://github.com/apache/incubator-airflow/blob/master/docs/conf.py#L18-L31

Reference: http://blog.rtwilson.com/how-to-make-your-sphinx-documentation-compile-with-readthedocs-when-youre-using-numpy-and-scipy/

@Fokko What Kaxil found is consistent with what I saw as well. There's a bit more info on it in (1) in my comment here #3703 (comment).

Just learned something, thanks @tedmiston !

kaxil

I have tested this and it works without having to add mock for readthedocs. Are you sure we need this @tedmiston ?

tedmiston · 2018-08-06T20:05:19Z

@ashb @kaxil Replying to all comments here since some are now hidden from lines changing in my force push.

I added a clarifying comment for the env var setting as requested.

So the mock dependency surprised me too... I sorta conflated 2 issues here. Here's what's happening.

1. When building the docs site locally, if I do a fresh install and try to build without mock in the docs extra.

$ SLUGIFY_USES_TEXT_UNIDECODE=yes pip install -e .[doc]
$ cd docs/
$ ./build.sh
sphinx-build -b html -d _build/doctrees   . _build/html
Running Sphinx v1.7.6

Configuration error:
There is a programable error in your configuration file:

Traceback (most recent call last):
  File "/Users/taylor/.local/share/virtualenvs/tmp-afb06f74a17a352/lib/python3.6/site-packages/sphinx/config.py", line 161, in __init__
    execfile_(filename, config)
  File "/Users/taylor/.local/share/virtualenvs/tmp-afb06f74a17a352/lib/python3.6/site-packages/sphinx/util/pycompat.py", line 150, in execfile_
    exec_(code, _globals)
  File "conf.py", line 16, in <module>
    import mock
ModuleNotFoundError: No module named 'mock'

make: *** [html] Error 2

This happens because conf.py for Sphinx imports mock on line 16 which it uses to build MOCK_MODULES. That appears to be a Sphinx pattern for a build system not having some module dependencies, which given the modules in that list, makes sense to me in a limited environment where I'm not doing a big install with more extras. Adding the mock change that's in this PR resolves that issue.

2. The CI issue fix... so if I try to replicate the 4 extras that I believe RTD is installing per .readthedocs.yml, with mock removed from the docs extra, then run the build script which effectively runs make html which effectively runs sphinx-build -b html . _build/html, I get the same error.

$ READTHEDOCS=True pip install -e .[doc,docker,gcp_api,emr]
$ cd docs/
$ ./build.sh
...same error as above...

This makes me think there might be something wonky with RTD. Perhaps it's running a different command than make html, or somehow is getting other extras installed that include mock (e.g., any of the extras that build on devel in setup.py). As far as I can tell from the RTD docs they are running the same command that errors out for me outside of their environment when I don't supply the mock dependency.

When we build your documentation, we run sphinx-build -b html . _build/html, where html would be replaced with the correct backend.

I'm all for getting to the bottom of the mock issue, but I also know that getting the docs site updating again is probably more urgent. If not having mock doesn't seem to affect RTD, I'm happy to remove it here.

So the mock issue is really a completely separate issue from the GPL RTD issue. Should I separate it from this PR? Or keep it here and add a comment? What do you think?

P.S. I also fixed some trailing whitespace in .readthedocs.yml.

feng-tao · 2018-08-06T21:01:04Z

I saw a different pr(#3701). Will that one solve the same issue that this pr tries to address? @tedmiston @ashb @kaxil

tedmiston · 2018-08-06T21:15:06Z

@feng-tao That PR makes a related change to the docker run command for local development, but doesn't cover the Read the Docs use case since RTD doesn't use those Docker commands. The main reason for the different workaround used here is that one cannot pass a custom env var into the RTD build environment as the Docker command does, so instead we set it based on an env var that they do provide.

Here's the mailing list thread from Kaxil with a bit more context on this PR - https://lists.apache.org/thread.html/3b7867b900bee8bdf013e6932812faa3ea891849c2b2eba000f0b093@%3Cdev.airflow.apache.org%3E.

ashb · 2018-08-07T08:09:43Z

Change the title of PR and Jira to "get RTD env working" or there abouts and I'm happy with the PR.

kaxil · 2018-08-07T14:21:07Z

Thanks @tedmiston for the fix and @ashb for the quick review. I have changed the title for the PR and Jira.

tedmiston · 2018-08-07T14:37:30Z

Thank you for the swift replies all! Excited to get this merged!

The Read the Docs build process was broken due to #3660. This PR fixes this. (cherry picked from commit 8af0aa9) Signed-off-by: Bolke de Bruin <[email protected]>

tedmiston · 2018-08-08T03:25:56Z

Hey all - It looks like our RTD build post-merge was successful. 😎

I dug into the mock dependency etc in the RTD build environment logs which led to creating a follow up Jira issue. If you have time to take a look and provide any comments there, it would be appreciated.

https://issues.apache.org/jira/browse/AIRFLOW-2871

The Read the Docs build process was broken due to apache#3660. This PR fixes this.

ashb reviewed Aug 6, 2018

View reviewed changes

kaxil requested changes Aug 6, 2018

View reviewed changes

[AIRFLOW-2857] Fix verify_gpl_dependency for Read the Docs

7007e95

kaxil changed the title ~~[AIRFLOW-2857] Fix verify_gpl_dependency for Read the Docs~~ [AIRFLOW-2857] Fix broken RTD env Aug 7, 2018

kaxil approved these changes Aug 7, 2018

View reviewed changes

kaxil merged commit 8af0aa9 into apache:master Aug 7, 2018

bolkedebruin pushed a commit that referenced this pull request Aug 7, 2018

[AIRFLOW-2857] Fix Read the Docs env (#3703)

c010e4a

The Read the Docs build process was broken due to #3660. This PR fixes this. (cherry picked from commit 8af0aa9) Signed-off-by: Bolke de Bruin <[email protected]>

bolkedebruin pushed a commit that referenced this pull request Aug 7, 2018

[AIRFLOW-2857] Fix Read the Docs env (#3703)

c764c11

The Read the Docs build process was broken due to #3660. This PR fixes this. (cherry picked from commit 8af0aa9) Signed-off-by: Bolke de Bruin <[email protected]>

tedmiston deleted the read-the-docs-gpl branch August 7, 2018 15:01

lxneng pushed a commit to lxneng/incubator-airflow that referenced this pull request Aug 10, 2018

[AIRFLOW-2857] Fix Read the Docs env (apache#3703)

96256c3

The Read the Docs build process was broken due to apache#3660. This PR fixes this.

aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019

[AIRFLOW-2857] Fix Read the Docs env (apache#3703)

570f90f

The Read the Docs build process was broken due to apache#3660. This PR fixes this.

amoghrajesh mentioned this pull request Mar 12, 2024

Migrating issue generation to breeze release-management #38062

Merged

[AIRFLOW-2857] Fix broken RTD env #3703

[AIRFLOW-2857] Fix broken RTD env #3703

Uh oh!

Conversation

tedmiston commented Aug 6, 2018

Jira

Description

Tests

Commits

Documentation

Code Quality

Uh oh!

tedmiston commented Aug 6, 2018

Uh oh!

codecov-io commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ashb Aug 6, 2018

Choose a reason for hiding this comment

Uh oh!

ashb Aug 6, 2018

Choose a reason for hiding this comment

Uh oh!

Fokko Aug 8, 2018

Choose a reason for hiding this comment

Uh oh!

kaxil Aug 8, 2018

Choose a reason for hiding this comment

Uh oh!

tedmiston Aug 8, 2018

Choose a reason for hiding this comment

Uh oh!

Fokko Aug 12, 2018

Choose a reason for hiding this comment

Uh oh!

kaxil left a comment

Choose a reason for hiding this comment

Uh oh!

tedmiston commented Aug 6, 2018

Uh oh!

feng-tao commented Aug 6, 2018

Uh oh!

tedmiston commented Aug 6, 2018

Uh oh!

ashb commented Aug 7, 2018

Uh oh!

kaxil commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tedmiston commented Aug 7, 2018

Uh oh!

tedmiston commented Aug 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Aug 6, 2018 •

edited

Loading

kaxil commented Aug 7, 2018 •

edited

Loading