Detect automatically the lack of reference to the guide in the operator descriptions #9290

ivan-afonichkin · 2020-06-14T14:24:18Z

Proposed change automatically detects lack of reference to the guide in the operator's description.

This is done by first checking if "class {operator_name}" is in the file. After that we build AST tree to check if there is really such a class defined. When the class is found, we just check if docstring contains ":ref:`howto/operator:{operator_name}`".

This implementation doesn't import operator's module, so no need to install dependencies.

Issue: #8894

boring-cyborg · 2020-06-14T14:24:21Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://apache-airflow-slack.herokuapp.com/

ashb

This is a -1 from me -- instead of having a separate guide page for each operator we should instead have better content on the API doc page for the operator itself.

Guides should be limited to case where we need to document/illustrate workflows that use more than a single operator.

And at any rate, this PR cannot be merged as you have just needed 31 new guides to be written.

ivan-transferwise · 2020-06-15T09:47:36Z

Hello @ashb, I believe there is misunderstanding on your side. What this PR does exactly is IF there is a separate guide, THEN require link from operator's docstring. IF there is no separate guide, THEN no link needed. Does it somehow clarify the situation?

You are absolutely right though that separate guides are not needed all the time, but when they exist, it's better to promote them by putting link in the docstring.

ashb · 2020-06-15T09:58:33Z

Gotcha, yes totally misread what it was doing. Sorry!

(That also explains why there were only 31 errors!)

I can't read

docs/build

mik-laj · 2020-06-16T14:00:25Z

airflow/operators/google_api_to_s3_transfer.py

-    `airflow.providers.amazon.aws.operators.google_api_to_s3_transfer.GoogleApiToS3TransferOperator`."""
+    `airflow.providers.amazon.aws.operators.google_api_to_s3_transfer.GoogleApiToS3TransferOperator`.
+
+    .. seealso::


This should not be added to this class. You can skip all files from the airflow.contrib directory.

But this is not under airflow.contrib. So why should it be skipped in this case?

It's a deprecated class. We will transfer many classes to the airflow.providers package. We only have references here to maintain backward compatibility.

Formerly the core code was maintained by the original creators - Airbnb. The code that was in the contrib package was supported by the community. The project was passed to the Apache community and currently the entire code is maintained by the community, so now the division has no justification, and it is only due to historical reasons.
https://airflow.readthedocs.io/en/stable/_api/index.html
We wanted to fix this, so we created a new airflow.providers package to organize these operators.
In the airflow.{operators,sensors} package, we only have a small set of core classes.

mik-laj · 2020-06-16T14:02:27Z

airflow/providers/amazon/aws/operators/ecs.py

@@ -101,6 +101,10 @@ class ECSOperator(BaseOperator):  # pylint: disable=too-many-instance-attributes
        Only required if you want logs to be shown in the Airflow UI after your job has
        finished.
    :type awslogs_stream_prefix: str
+
+    .. seealso::


It would be fantastic if the parameter description was at the end of the class description. The link to the guide should be above the description of the parameters. Look how it is done in airflow.providers.google

Here is example: https://airflow.readthedocs.io/en/latest/_api/airflow/providers/google/cloud/operators/cloud_storage_transfer_service/index.html#airflow.providers.google.cloud.operators.cloud_storage_transfer_service.CloudDataTransferServiceCreateJobOperator
When this is under the parameters description, it will not fulfill its role, because it will be hardly visible. This will not promote guides.

mik-laj · 2020-06-16T19:36:05Z

docs/build

+    for py_module_path in python_module_paths:
+        with open(py_module_path) as f:
+            py_content = f.read()
+        for existing_operator in operator_names:


Suggested change

for existing_operator in operator_names:

if "This module is deprecated" in py_content:

continue

for existing_operator in operator_names:

mik-laj · 2020-06-16T19:40:36Z

docs/build

+            # Real class definition is found and docstring does not contain reference to the existing guide
+            if class_def is not None and f":ref:`howto/operator:{existing_operator}`" not in ast.get_docstring(class_def):


Suggested change

# Real class definition is found and docstring does not contain reference to the existing guide

if class_def is not None and f":ref:`howto/operator:{existing_operator}`" not in ast.get_docstring(class_def):

# Real class definition is not found

if class_def is None:

continue

doc = ast.get_docstring(class_def)

if "This class is deprecated." in docstring:

continue

if f":ref:`howto/operator:{existing_operator}`" in doc:

continue

The documentation must build properly for this change to be accepted. In some places we don't want links, so we have to handle it in the code.

mik-laj · 2020-06-16T19:46:32Z

Could you wait with your change until the other change is merged? Otherwise, we will have a lot of conflicts, which can be a bit problematic. This change is needed to release backport packages, so many users hope that we will finish it ASAP.
#9320

potiuk · 2020-06-17T08:33:26Z

The transfer package change is merged now - so please rebase. those "to" operators moved to "transfers" package.

ivan-afonichkin · 2020-06-17T10:17:33Z

Thank you everyone for the input, especially @mik-laj! 👍

Will do all these fixes later today.

mik-laj · 2020-06-17T17:05:25Z

@ivan-afonichkin Ohh. CI is still sad. Can you fix it?

ivan-afonichkin · 2020-06-17T17:37:29Z

@mik-laj Yes, I will do it, just didn't have time to fix everything. I will be back home in 3-4h and fix it :)

mik-laj · 2020-06-17T20:22:17Z

docs/build

+        glob(f"{ROOT_PACKAGE_DIR}/operators/*.py"),
+        glob(f"{ROOT_PACKAGE_DIR}/sensors/*.py"),
+        glob(f"{ROOT_PACKAGE_DIR}/providers/**/operators/*.py", recursive=True),
+        glob(f"{ROOT_PACKAGE_DIR}/providers/**/sensors/*.py", recursive=True),


Can you add transfers package here also?

ivan-afonichkin · 2020-06-17T21:43:25Z

Hey @mik-laj, CI Build / Build docs has finally passed, but some databases checks failed with no apparent reason to me, any idea what could be wrong there?

mik-laj · 2020-06-17T21:55:39Z

@ivan-afonichkin It looks like one of the changes that I merged had a defect. I'm already looking at it.

mik-laj · 2020-06-17T22:21:01Z

@kaxil you self-requested a review 3 days ago. Would you like to add something here?

mik-laj · 2020-06-17T23:13:01Z

@ivan-afonichkin Fix has been merged to master. Can you do a rebase?

…or descriptions

Check specific folders for operators/sensors Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

…iptions function Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

…or descriptions

Check specific folders for operators/sensors Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

…iptions function Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

ivan-afonichkin · 2020-06-18T07:57:33Z

Hey @mik-laj it seems all checks have passed, thanks for your change! :)
Can we merge this PR?

boring-cyborg · 2020-06-18T09:33:16Z

Awesome work, congrats on your first merged pull request!

mik-laj · 2020-06-18T09:54:13Z

@ivan-afonichkin Thank you for a great contribution. This will make our work much easier, because documentation reviews will be easier. What are your plans for the next change?

ivan-afonichkin · 2020-06-18T10:01:50Z

@mik-laj Thanks a lot, very glad it was merged, and thanks a lot for all the help and support!
Regarding the next change, I didn't choose an issue yet, but maybe you have something on your mind that will be very beneficial and needs to be done? If not, I will just go through the backlog :)

mik-laj · 2020-06-18T10:25:01Z

@mik-laj Your experience with AST would probably be helpful with this ticket, but we still need to do a few things before we can implement it.
#8765
If we can set the details and unblock the work, I will call you If you or your company uses Airflow in production, then maybe you would like to share your opinion in this thread
#9276
After that, we don't have an idea for a ticket that might be of interest to you. However, I create all new tickets all the time, so you will definitely find something for you.

…or descriptions (apache#9290) Co-authored-by: ivan.afonichkin <ivan.afonichkin@transferwise.com> Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

boring-cyborg bot added the area:docs label Jun 14, 2020

ivan-afonichkin mentioned this pull request Jun 14, 2020

Detect automatically the lack of reference to the guide in the operator descriptions #8894

Closed

mik-laj self-requested a review June 14, 2020 15:17

kaxil self-requested a review June 14, 2020 15:25

ashb previously requested changes Jun 15, 2020

View reviewed changes

mik-laj reviewed Jun 15, 2020

View reviewed changes

docs/build Outdated Show resolved Hide resolved

mik-laj reviewed Jun 15, 2020

View reviewed changes

docs/build Outdated Show resolved Hide resolved

mik-laj reviewed Jun 15, 2020

View reviewed changes

docs/build Outdated Show resolved Hide resolved

mik-laj reviewed Jun 16, 2020

View reviewed changes

ivan-afonichkin force-pushed the reference-lack-operator-description branch from c744406 to 7ecbab1 Compare June 16, 2020 14:27

mik-laj reviewed Jun 16, 2020

View reviewed changes

ivan-afonichkin force-pushed the reference-lack-operator-description branch from 1e894a0 to 1023771 Compare June 17, 2020 15:53

mik-laj reviewed Jun 17, 2020

View reviewed changes

mik-laj approved these changes Jun 17, 2020

View reviewed changes

kaxil approved these changes Jun 17, 2020

View reviewed changes

ivan-transferwise and others added 14 commits June 18, 2020 08:03

Detect automatically the lack of reference to the guide in the operat…

04a1158

…or descriptions

Update docs/build

bd7b912

Check specific folders for operators/sensors Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

Fix too many indentations in docs check_guide_links_in_operator_descr…

a2122be

…iptions function Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

Better formatting for docstring guide snippet

d057082

Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

Updated operator's docs (guide link is in the header of docstring)

e4eb106

itertools -> chain

818bcd0

Detect automatically the lack of reference to the guide in the operat…

705254e

…or descriptions

Update docs/build

4b3a327

Check specific folders for operators/sensors Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

Fix too many indentations in docs check_guide_links_in_operator_descr…

250a839

…iptions function Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

Better formatting for docstring guide snippet

98b1647

Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

itertools -> chain

d191571

Skip deprecated modules and classes for docstrings in operators

279205f

Added transfers folder to look for operators in docs/build

9c5b57b

Headers for HttpSensor and SimpleHttpOperator in documentation

f498c04

ivan-afonichkin force-pushed the reference-lack-operator-description branch from 8a27fc6 to f498c04 Compare June 18, 2020 07:04

mik-laj approved these changes Jun 18, 2020

View reviewed changes

mik-laj merged commit 40bf8f2 into apache:master Jun 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect automatically the lack of reference to the guide in the operator descriptions #9290

Detect automatically the lack of reference to the guide in the operator descriptions #9290

ivan-afonichkin commented Jun 14, 2020 •

edited

Loading

boring-cyborg bot commented Jun 14, 2020

ashb left a comment

ivan-transferwise commented Jun 15, 2020

ashb commented Jun 15, 2020

mik-laj Jun 16, 2020

ivan-afonichkin Jun 16, 2020

mik-laj Jun 16, 2020

mik-laj Jun 16, 2020

mik-laj Jun 16, 2020

mik-laj Jun 16, 2020

mik-laj Jun 16, 2020

mik-laj Jun 16, 2020

mik-laj Jun 16, 2020

mik-laj commented Jun 16, 2020

potiuk commented Jun 17, 2020

ivan-afonichkin commented Jun 17, 2020

mik-laj commented Jun 17, 2020

ivan-afonichkin commented Jun 17, 2020

mik-laj Jun 17, 2020

ivan-afonichkin commented Jun 17, 2020

mik-laj commented Jun 17, 2020

mik-laj commented Jun 17, 2020

mik-laj commented Jun 17, 2020

ivan-afonichkin commented Jun 18, 2020

boring-cyborg bot commented Jun 18, 2020

mik-laj commented Jun 18, 2020

ivan-afonichkin commented Jun 18, 2020

mik-laj commented Jun 18, 2020

		# Real class definition is found and docstring does not contain reference to the existing guide
		if class_def is not None and f":ref:`howto/operator:{existing_operator}`" not in ast.get_docstring(class_def):

-            # Real class definition is found and docstring does not contain reference to the existing guide
-            if class_def is not None and f":ref:`howto/operator:{existing_operator}`" not in ast.get_docstring(class_def):
+            # Real class definition is not found
+            if class_def is None:
+                continue
+            doc = ast.get_docstring(class_def)
+            if "This class is deprecated." in docstring:
+                continue
+            if f":ref:`howto/operator:{existing_operator}`" in doc:
+                continue

Detect automatically the lack of reference to the guide in the operator descriptions #9290

Detect automatically the lack of reference to the guide in the operator descriptions #9290

Conversation

ivan-afonichkin commented Jun 14, 2020 • edited Loading

boring-cyborg bot commented Jun 14, 2020

ashb left a comment

Choose a reason for hiding this comment

ivan-transferwise commented Jun 15, 2020

ashb commented Jun 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-laj commented Jun 16, 2020

potiuk commented Jun 17, 2020

ivan-afonichkin commented Jun 17, 2020

mik-laj commented Jun 17, 2020

ivan-afonichkin commented Jun 17, 2020

Choose a reason for hiding this comment

ivan-afonichkin commented Jun 17, 2020

mik-laj commented Jun 17, 2020

mik-laj commented Jun 17, 2020

mik-laj commented Jun 17, 2020

ivan-afonichkin commented Jun 18, 2020

boring-cyborg bot commented Jun 18, 2020

mik-laj commented Jun 18, 2020

ivan-afonichkin commented Jun 18, 2020

mik-laj commented Jun 18, 2020

ivan-afonichkin commented Jun 14, 2020 •

edited

Loading