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.

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

Executors doc update #34324

Merged
merged 5 commits into from Oct 10, 2023
Merged

Executors doc update #34324

merged 5 commits into from Oct 10, 2023

Conversation

o-nikolas
Copy link
Contributor

@o-nikolas o-nikolas commented Sep 13, 2023

Added new sections describing the BaseExecutor interface in more detail to help users in writing custom executors.

This addresses the last AIP-51 task #27934 as well as #33146

A screenshot of the fully rendered page can be viewed below:
Screenshot 2023-09-27 at 12-41-21 Executor — Airflow Documentation


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Added sections describing the BaseExecutor interface in more detail
to help in writing custom executors
@boring-cyborg boring-cyborg bot added area:Executors-core LocalExecutor & SequentialExecutor kind:documentation labels Sep 13, 2023
@o-nikolas
Copy link
Contributor Author

CC @eladkal @potiuk and @Bowrna who showed interested in this task

@potiuk
Copy link
Member

potiuk commented Sep 13, 2023

Small comment - not directly this but related and clearly visible in the rendered page. Can you please move the "Testing" documentation out of the executor hierarchy ? The two Howtos seems completely out of place for the document.

I like the document. The few proposals I would like to propose:

  • Instead of "strongly recommend" I would write "Sequential Executor is not suitable for production. It does not allow for parallel task runnning and due to that, some Airflow features (for example running sensors waiting for outputs of another task are not working properly).

  • Maybe it would be possible to Have a boilerplate template of an executor as part of the docs ? That would probably significantly decrease the friction of writing new executor (on the other hand, maybe we actually want some friction). Writing your own executor is not an easy / simple task that everyone should even attempt

  • Speaking of which - I think we should explaine why somoene would like to write their own executor. Short paragraph describing a decision making process here and some example reasons why one would rather write their own executor than choose from the existing ones, would be great.

@o-nikolas
Copy link
Contributor Author

o-nikolas commented Sep 13, 2023

Thanks for the feedback @potiuk, replies inline below :)

Small comment - not directly this but related and clearly visible in the rendered page. Can you please move the "Testing" documentation out of the executor hierarchy ? The two Howtos seems completely out of place for the document.

I don't see a "Testing" doc in the executor hierarchy? I'm likely missing something though 👀

I like the document. The few proposals I would like to propose:

Instead of "strongly recommend" I would write "Sequential Executor is not suitable for production. It does not allow for parallel task runnning and due to that, some Airflow features (for example running sensors waiting for outputs of another task are not working properly).

This section actually already existed before I updated it (my changes start at "Writing Your Own Executor"), but I can make this change anyway while I'm here.

Maybe it would be possible to Have a boilerplate template of an executor as part of the docs ? That would probably significantly decrease the friction of writing new executor (on the other hand, maybe we actually want some friction). Writing your own executor is not an easy / simple task that everyone should even attempt

I did think of this, but unlike custom operators, it's quite a large piece of code to even have a boilerplate. I was thinking of adding a very pseudo code snippet, but wasn't convinced that it would be helpful beyond folks just having a look at any of the existing community executors which all implement the BaseExecutor interface. Interested to hear what others think!

Speaking of which - I think we should explaine why somoene would like to write their own executor. Short paragraph describing a decision making process here and some example reasons why one would rather write their own executor than choose from the existing ones, would be great.

Great suggestion. I'll add a section and come up with a list of bullets for why you'd want to create a custom executor. If you have any to add, please reply with them here!

@potiuk
Copy link
Member

potiuk commented Sep 13, 2023

I don't see a "Testing" doc in the executor hierarchy? I'm likely missing something though 👀

Screenshot 2023-09-13 at 23 06 11

What I see:

  • Testing ....
  • Debugging ....
  • DebugExecutor (Deprecated)

I think it's because those files are wrongly placed :)

This section actually already existed before I updated it (my changes start at "Writing Your Own Executor"), but I can make this change anyway while I'm here.

Yep. It's all "Let's make the doc complete and makes sensee" rather than "Let's preserve what was wrong before"

I did think of this, but unlike custom operators, it's quite a large piece of code to even have a boilerplate. I was thinking of adding a very pseudo code snippet, but wasn't convinced that it would be helpful beyond folks just having a look at any of the existing community executors which all implement the BaseExecutor interface. Interested to hear what others think!

Agree. It should be enough to say ("look at existing executor's implementation to take inspiration" - which I think is already there, so unless there is some nicer way to surface, it's more of a "nice to have".

I'll add a section and come up with a list of bullets for why you'd want to create a custom executor. If you have any to add, please reply with them here!

Will do !

@o-nikolas
Copy link
Contributor Author

I don't see a "Testing" doc in the executor hierarchy? I'm likely missing something though 👀

Screenshot 2023-09-13 at 23 06 11

What I see:

* Testing ....

* Debugging ....

* DebugExecutor (Deprecated)

I think it's because those files are wrongly placed :)

Ah! In the types subsection. I was looking for it in the table of contents hierarchy on the left.

Those three sections all come from the DebugExecutor doc. Which has been modified to move the section on the DebugExecutor itself to the bottom of the page and new sections were added for testing dags without the debug executor (now that it's deprecated), and some comparisons with the debug executor itself. It's a little tricky now how to handle that with the .. toctree:: directive we use for those lists.

The easiest thing may be to just remove the debug executor from the .. toctree:: directive.

@o-nikolas
Copy link
Contributor Author

@potiuk

Changes are in! It was quite a sticky mess with the debug executor doc. The fix I landed on was just shuffling around the headings on that page to not have 3 title level headings (which actually makes sense, I think that was a bug before). And move the DebugExecutor section to the top (which makes sense, since it is the DebugExecutor doc page, the other info is additional).

Let me know what you think.

@syedahsn
Copy link
Contributor

Looks really good. Very well done! Just a comment, and I'm not sure how much can be done about this, but the Logging and CLI sections don't really add a lot of information about how to log or vend CLI commands. I don't know what the process for that is, and whether that's something that should be discussed in a document like this. But if a bit more information (or links to examples from the LocalExecutor or Kubernetes Executor) can be added about that, it would help I think.

@o-nikolas
Copy link
Contributor Author

Looks really good. Very well done! Just a comment, and I'm not sure how much can be done about this, but the Logging and CLI sections don't really add a lot of information about how to log or vend CLI commands. I don't know what the process for that is, and whether that's something that should be discussed in a document like this. But if a bit more information (or links to examples from the LocalExecutor or Kubernetes Executor) can be added about that, it would help I think.

Thanks for having a look @syedahsn

I can try add a bit more info. But so far I've just mentioned which executors make use of it, rather than linking directly to code, since that link will very quickly become stale as changes are made to that class. Maybe I can put some pseudo code in here?

@o-nikolas
Copy link
Contributor Author

@syedahsn

I added some pseudo code to the CLI and logging sections to give a more concrete idea of how to use those parts of the interface.

@@ -15,8 +15,47 @@
specific language governing permissions and limitations
under the License.

.. _executor:DebugExecutor:
Copy link
Contributor Author

@o-nikolas o-nikolas Sep 27, 2023

Choose a reason for hiding this comment

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

No changes here other than bringing this section to the top and modifying section hierarchy to satisfy the static checks we have.

Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

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

Just a few style/grammar suggestions. Otherwise LGTM.

docs/apache-airflow/core-concepts/executor/index.rst Outdated Show resolved Hide resolved
docs/apache-airflow/core-concepts/executor/index.rst Outdated Show resolved Hide resolved
docs/apache-airflow/core-concepts/executor/index.rst Outdated Show resolved Hide resolved
docs/apache-airflow/core-concepts/executor/index.rst Outdated Show resolved Hide resolved
docs/apache-airflow/core-concepts/executor/index.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@o-nikolas
Copy link
Contributor Author

This has been open for a while and the feedback has stopped trickling in. @potiuk, @vincbeck, @syedahsn, @ferruzzi I think I've addressed all your feedback. Merging this one!

@o-nikolas o-nikolas merged commit 8fdf358 into apache:main Oct 10, 2023
42 checks passed
@o-nikolas o-nikolas deleted the onikolas/executor_docs branch October 10, 2023 18:47
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.3 milestone Oct 27, 2023
@ephraimbuddy ephraimbuddy added the type:doc-only Changelog: Doc Only label Oct 27, 2023
ephraimbuddy pushed a commit that referenced this pull request Oct 29, 2023
* Executors doc update

Added sections describing the BaseExecutor interface in more detail
to help in writing custom executors

(cherry picked from commit 8fdf358)
ephraimbuddy pushed a commit that referenced this pull request Oct 30, 2023
* Executors doc update

Added sections describing the BaseExecutor interface in more detail
to help in writing custom executors

(cherry picked from commit 8fdf358)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Executors-core LocalExecutor & SequentialExecutor kind:documentation type:doc-only Changelog: Doc Only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants