mirror of
https://github.com/apache/airflow.git
synced 2026-11-04 07:12:48 +00:00
A docs-only change under dev/breeze/ drafted the pull request and waited for a maintainer as if it changed CI, although only the files that decide which jobs run and on which runners (workflows, breeze sources, the breeze installation, and scripts/ci/prek/ whose provider dependencies script selective checks runs) can let a pull request steer its own CI. Such pull requests still wait for a maintainer to run project CI, but are no longer drafted, so they stay visible for review. Changes to the other files CI uses are no longer held, but are still pointed out to the reviewer in a one-time comment. The provider dependencies script now ignores uv configuration from the checkout, so a pull request cannot redirect where its dependencies come from. Generated-by: Claude Opus 5
598 lines
32 KiB
ReStructuredText
598 lines
32 KiB
ReStructuredText
contributing-docs/05_pull_requests.rst
|
|
.. Licensed to the Apache Software Foundation (ASF) under one
|
|
or more contributor license agreements. See the NOTICE file
|
|
distributed with this work for additional information
|
|
regarding copyright ownership. The ASF licenses this file
|
|
to you under the Apache License, Version 2.0 (the
|
|
"License"); you may not use this file except in compliance
|
|
with the License. You may obtain a copy of the License at
|
|
|
|
.. http://www.apache.org/licenses/LICENSE-2.0
|
|
|
|
.. Unless required by applicable law or agreed to in writing,
|
|
software distributed under the License is distributed on an
|
|
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
|
|
KIND, either express or implied. See the License for the
|
|
specific language governing permissions and limitations
|
|
under the License.
|
|
|
|
Pull Requests
|
|
=============
|
|
|
|
This document describes how you can create Pull Requests (PRs) and describes coding standards we use when
|
|
implementing them.
|
|
|
|
.. contents:: Table of Contents
|
|
:depth: 2
|
|
:local:
|
|
|
|
Protect your commit identity
|
|
----------------------------
|
|
|
|
Once a PR is merged, the commit author's (and any co-author's) name and email address become
|
|
**permanently public** as part of the commit history and **cannot be changed or removed afterward**.
|
|
|
|
GitHub provides a **built-in option to anonymize your commit email address**
|
|
(`Setting your commit email address <https://docs.github.com/en/account-and-profile/how-tos/email-preferences/setting-your-commit-email-address>`__)
|
|
by using a GitHub-provided email; however, **this setting applies only to future commits and cannot be
|
|
applied retrospectively**.
|
|
|
|
On computers used with **multiple GitHub accounts** (for example, shared or work computers), the local Git
|
|
configuration may be set to a different name or email than expected. This can result in unintended personal
|
|
or work-related details being permanently published. If you are using the Git CLI, you can verify your
|
|
current configuration by running the following commands in your terminal within your local Airflow repository:
|
|
|
|
.. code-block:: bash
|
|
|
|
git config --get user.name
|
|
git config --get user.email
|
|
|
|
To keep your email address private on GitHub, enable *Keep my email addresses private* in your GitHub
|
|
settings and configure git to use your GitHub-provided ``noreply`` address (typically
|
|
``<id>+<username>@users.noreply.github.com``) as ``user.email`` for future commits.
|
|
|
|
If you use the Git CLI, you can set the appropriate configuration by running the following commands:
|
|
|
|
.. code-block:: bash
|
|
# Replace <id> and <username> accordingly with your GitHub user ID and username (from the GitHub settings page)
|
|
git config user.email "<id>+<username>@users.noreply.github.com"
|
|
|
|
# Replace "Your Name" with your desired commit author name
|
|
git config user.name "Your Name"
|
|
|
|
Notes:
|
|
|
|
* GitHub's anonymization does **not** apply to co-author entries or to email addresses explicitly included
|
|
in the commit message.
|
|
* GitHub Desktop and other third-party Git clients manage author and email settings separately; verify
|
|
them using the tool's own configuration or preferences.
|
|
|
|
Before opening a pull request, please ensure you are comfortable with the author and co-author details that
|
|
will be publicly visible. Maintainers might point out unexpected personal information before merging,
|
|
but they are not obliged to do so.
|
|
|
|
Pull Request guidelines
|
|
-----------------------
|
|
|
|
Before you submit a Pull Request (PR) from your forked repo, check that it meets
|
|
these guidelines:
|
|
|
|
- Mind the **limit on open PRs**: contributors without write access can have at most 5 open PRs
|
|
at a time. See `Limit on open Pull Requests <32_open_pull_request_limit.rst>`__ for why, and what
|
|
to do when you reach it.
|
|
|
|
- Start with **Draft**: Until you are sure that your PR passes all the quality checks and tests, keep it
|
|
in **Draft** status. This will signal to maintainers that the PR is not yet ready
|
|
for review and it will prevent maintainers from accidentally merging it before
|
|
it's ready. Once you are sure that your PR is ready for review, you can mark it as
|
|
"Ready for review" in the GitHub UI. Our regular check will convert all PRs from
|
|
non-collaborators that do not pass our quality gates to Draft status, so if you see
|
|
that your PR is in Draft status and you haven't set it to Draft. Check the
|
|
comments to see what needs to be fixed.
|
|
|
|
- Include tests, either as doctests, unit tests, or both, to your pull request.
|
|
|
|
The Airflow repo uses `GitHub Actions <https://help.github.com/en/actions>`_ to
|
|
run the tests and `codecov <https://codecov.io/gh/apache/airflow>`_ to track
|
|
coverage. Fork CI is being piloted: it applies to pilot participants and to pull requests a
|
|
maintainer labels ``use fork ci``, and to every external contributor only once it is switched
|
|
on for everyone. Until then, enabling the ``Tests (AMD)`` workflow in your fork outside the
|
|
pilot runs CI twice for each pull request, in your fork and in apache/airflow. When fork CI
|
|
applies to you, enable that workflow and every push to your fork runs CI for free; pull
|
|
requests labelled ``fork ci required`` take their CI result from your fork and stay in draft
|
|
until that run is green. See `CI in your fork <18_contribution_workflow.rst#ci-in-your-fork>`__
|
|
and run ``breeze ci audit``, which tells you whether fork CI applies to you yet.
|
|
Also we advise to install locally `prek hooks <08_static_code_checks.rst#prek-hooks>`_ to
|
|
apply various checks, code generation and formatting at the time you make a local commit - which
|
|
gives you near-immediate feedback on things you need to fix before you push your code to the PR, or in
|
|
many case it will even fix it for you locally so that you can add and commit it straight away.
|
|
|
|
- Follow our project's `Coding style and best practices`_. Usually we attempt to enforce the practices by
|
|
having appropriate prek hooks. There are checks amongst them that aren't currently enforced
|
|
programmatically (either because they are too hard or just not yet done).
|
|
|
|
- Maintainers will not merge a PR that regresses linting or does not pass CI tests (unless you have good
|
|
justification that it is a transient error or something that is being fixed in other PR).
|
|
|
|
- Maintainers will not merge PRs that have unresolved conversation.
|
|
|
|
- We prefer that you ``rebase`` your PR (and do it quite often) rather than merge. It leads to
|
|
easier reviews and cleaner changes where you know exactly what changes you've done. You can learn more
|
|
about rebase vs. merge workflow in `Rebase and merge your pull request <https://github.blog/2016-09-26-rebase-and-merge-pull-requests/>`_
|
|
and `Rebase your fork <http://stackoverflow.com/a/7244456/1110993>`_. Make sure to resolve all conflicts
|
|
during rebase.
|
|
|
|
- When merging PRs, Maintainer will use **Squash and Merge** which means then your PR will be merged as one
|
|
commit, regardless of the number of commits in your PR. During the review cycle, you can keep a commit
|
|
history for easier review, but if you need to, you can also squash all commits to reduce the
|
|
maintenance burden during rebase.
|
|
|
|
- Add an `Apache License <http://www.apache.org/legal/src-headers.html>`_ header to all new files. If you
|
|
have ``prek`` installed, prek will do it automatically for you. If you hesitate to install
|
|
prek for your local repository - for example because it takes a few seconds to commit your changes,
|
|
this one thing might be a good reason to convince anyone to install prek.
|
|
|
|
- If your PR adds functionality, make sure to update the docs as part of the same PR, not only
|
|
code and tests. Docstring is often sufficient. Make sure to follow the Sphinx compatible standards.
|
|
|
|
- Make sure your code fulfills all the
|
|
`static code checks <08_static_code_checks.rst#static-code-checks>`_ we have in our code. The easiest way
|
|
to make sure of that is - again - to install `prek hooks <08_static_code_checks.rst#prek-hooks>`_.
|
|
|
|
- Make sure your PR is small and focused on one change only - avoid adding unrelated changes, mixing
|
|
adding features and refactoring. Keeping to that rule will make it easier to review your PR and will make
|
|
it easier for release managers if they decide that your change should be cherry-picked to release it in a
|
|
bug-fix release of Airflow. If you want to add a new feature and refactor the code, it's better to split the
|
|
PR to several smaller PRs. It's also quite a good and common idea to keep a big ``Draft`` PR if you have
|
|
a bigger change that you want to make and then create smaller PRs from it that are easier to review and
|
|
merge and cherry-pick. It takes a long time (and a lot of attention and focus of a reviewer to review
|
|
big PRs so by splitting it to smaller PRs you actually speed up the review process and make it easier
|
|
for your change to be eventually merged.
|
|
|
|
- To learn more about ``cherry-pick`` of PRs, refer to the `Airflow 3 development documentation <../blob/main/dev/README_AIRFLOW3_DEV.md>`_.
|
|
|
|
- Run relevant tests locally before opening PR. Often tests are placed in the files that are corresponding
|
|
to the changed code (for example for ``airflow/cli/cli_parser.py`` changes you have tests in
|
|
``tests/cli/test_cli_parser.py``). However there are a number of cases where the tests that should run
|
|
are placed elsewhere - you can either run tests for the whole ``TEST_TYPE`` that is relevant (see
|
|
``breeze testing core-tests --help`` or ``breeze testing providers-tests --help`` output for
|
|
available test types for each of the testing commands) or you can run all tests, or eventually
|
|
you can push your code to PR and see results of the tests in the CI.
|
|
|
|
- You can use any supported python version to run the tests, but the best is to check
|
|
if it works for the oldest supported version (Python 3.11 currently). In rare cases
|
|
tests might fail with the oldest version when you use features that are available in newer Python
|
|
versions. For that purpose we have ``airflow.compat`` package where we keep back-ported
|
|
useful features from newer versions.
|
|
|
|
- Adhere to guidelines for commit messages described in this `article <https://cbea.ms/git-commit/>`_.
|
|
This makes the lives of those who come after you (and your future self) a lot easier.
|
|
|
|
.. _pull-request-quality-criteria:
|
|
|
|
Pull Request quality criteria
|
|
-----------------------------
|
|
|
|
Every open PR must meet the following minimum quality criteria before maintainers will review it.
|
|
PRs that do not meet these criteria may be automatically converted to **draft** status by project
|
|
tooling, with a comment explaining what needs to be fixed.
|
|
|
|
1. **Descriptive title** — The PR title must clearly describe the change.
|
|
Generic titles such as "Fix bug", "Update code", "Changes", single-word titles, or titles
|
|
that only reference an issue number (e.g. "Fixes #12345") do not meet this bar.
|
|
|
|
2. **Golden Rule** - in text descriptions - always use imperative mode, never use past
|
|
tense or conventional commits or any prefixes in the title take precious space and do not add any value, so they should be
|
|
avoided. For example, instead of "feat: add new feature" or "Added this new feature"
|
|
use "Add new feature". This is a common convention in many open source projects
|
|
and it makes the commit history more readable and consistent.
|
|
|
|
3. **Meaningful description** — The PR body must contain a meaningful description of *what* the
|
|
PR does and *why*. An empty body, a body consisting only of the PR template
|
|
checkboxes/headers with no added text, or a body that merely repeats the title is not
|
|
sufficient.
|
|
|
|
4. **Passing static checks** — The PR's static checks (pre-commit / ruff / mypy) must pass.
|
|
You can run them locally with ``prek run --from-ref main`` before pushing.
|
|
|
|
5. **Gen-AI disclosure** — If the PR was created with the assistance of generative AI tools,
|
|
the description must include a disclosure (see `Gen-AI Assisted contributions`_ below).
|
|
|
|
6. **Coherent changes** — The PR should contain related changes only. Completely unrelated
|
|
changes bundled together will be flagged.
|
|
|
|
**What happens when a PR is converted to draft?**
|
|
|
|
- The comment informs you what you need to do.
|
|
- Fix each issue, then mark the PR as "Ready for review" in the GitHub UI - but only after making sure that all the issues are fixed.
|
|
- There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates.
|
|
- Maintainers will then proceed with a normal review.
|
|
|
|
Converting a PR to draft is **not** a rejection — it is an invitation to bring the PR up to
|
|
the project's standards so that maintainer review time is spent productively.
|
|
|
|
For details on how maintainers use tooling to triage and review PRs, see
|
|
`Maintainer PR Triage and Review <25_maintainer_pr_triage.md>`__.
|
|
|
|
**What happens when a PR is closed for quality violations?**
|
|
|
|
If a contributor has more than 3 open PRs that are flagged for quality issues, maintainers may
|
|
choose to **close** the PR instead of converting it to draft. Closed PRs receive the
|
|
``closed because of multiple quality violations`` label and a comment listing the violations.
|
|
Contributors are welcome to open a new PR that addresses the issues listed in the comment.
|
|
|
|
**What happens when suspicious changes are detected?**
|
|
|
|
When maintainers review a PR's diff before approving CI workflow runs and determine that it
|
|
contains suspicious changes (e.g. attempts to exfiltrate secrets, modify CI pipelines
|
|
maliciously, or inject harmful code), **all open PRs by the same author** will be closed
|
|
and labeled ``suspicious changes detected``. A comment is posted on each PR explaining that
|
|
the closure was triggered by suspicious changes found in the flagged PR.
|
|
|
|
**What happens when a PR is closed because of the open PR limit?**
|
|
|
|
When the `limit on open PRs <32_open_pull_request_limit.rst>`__ was introduced, maintainers ran a
|
|
one-time closure of the PRs of contributors who had more than 5 open PRs, except PRs where a
|
|
maintainer had already engaged. Those PRs are labeled ``closed because of open PR limit`` and can be
|
|
reopened once you have a free slot.
|
|
|
|
If a contributor believes any closure described above was applied in error, the appeal channel
|
|
is the PMC private list — see the
|
|
`Community escalation process <../COMMUNITY_ESCALATION.md>`_ for how to email
|
|
``private@airflow.apache.org``. That document also covers the further steps the project may
|
|
take when the same behaviour persists across PRs.
|
|
|
|
CI workflow runs of external contributors
|
|
-----------------------------------------
|
|
|
|
GitHub does not start the CI workflows of a PR from a fork until a maintainer approves them,
|
|
for the contributors that the repository's approval setting covers. To keep you from waiting
|
|
for a maintainer on every push, a scheduled workflow checks the waiting runs every 15 minutes
|
|
and approves them automatically when all of these hold:
|
|
|
|
* The PR targets ``main``. PRs to release branches, such as ``v3-4-test``, always wait for a
|
|
maintainer.
|
|
* The PR does not change files that decide which CI jobs run and on which runners: anything under
|
|
``.github/``, ``dev/breeze/src/`` or ``scripts/ci/prek/``, or ``dev/breeze/pyproject.toml``,
|
|
``dev/breeze/uv.lock`` or ``scripts/ci/install_breeze.sh``. Changes to the other files CI uses
|
|
(the rest of ``dev/breeze/`` and ``scripts/ci/``, ``scripts/docker/``, ``scripts/in_container/``,
|
|
``Dockerfile*``, ``.pre-commit-config.yaml``) are not held, but a one-time comment on the PR
|
|
points them out to the reviewer.
|
|
* The change does not need the full set of tests, does not change dependencies (``pyproject.toml``,
|
|
lock files, ``package.json``, ``go.mod``, Gradle build files) and does not add a new provider.
|
|
* The CI jobs your change selects are estimated to take at most 310 job-minutes, about a quarter of
|
|
a full CI run. Changes to documentation, to the UI, to the Helm chart, to the language SDKs, to
|
|
one area of Airflow core or to a small provider usually stay under it. Changes to large providers
|
|
such as Amazon or Google, to several providers, or to the Task SDK (which runs the tests of all
|
|
providers) usually do not.
|
|
* The PR changes at most 100 lines of source code. Tests (including end-to-end tests),
|
|
documentation, newsfragments, translations and lock files are not counted, so a fix with
|
|
thorough tests still qualifies. Larger changes are usually new features, and a maintainer looks
|
|
at them first.
|
|
|
|
If your PR's CI runs in your fork (it carries the ``fork ci required`` label), the CI run in
|
|
``apache/airflow`` only checks that, so none of the rules above apply. The ``CI run started`` check
|
|
waits until the fork CI monitor sees your fork's run, usually within 15 minutes, and then links it. Its runs are approved
|
|
automatically unless the PR changes files that decide which CI jobs run and on which runners:
|
|
anything under ``.github/``, ``dev/breeze/src/`` or ``scripts/ci/prek/``, or
|
|
``dev/breeze/pyproject.toml``, ``dev/breeze/uv.lock`` or ``scripts/ci/install_breeze.sh``. Then
|
|
the runs wait for a maintainer and a comment on the PR explains why. Changes to the other files
|
|
CI uses are not held, but a one-time comment points them out to the reviewer. CI in your fork
|
|
runs either way.
|
|
|
|
The runs of PRs opened from the repositories of trusted stakeholders (``astronomer/airflow``,
|
|
``aws-mwaa/upstream-to-airflow`` and ``VladaZakharova/airflow``) are always approved, so none of
|
|
the rules above apply to them.
|
|
|
|
Each new push is checked again, because approval applies to one commit. Only runs started in the
|
|
last 24 hours are considered, so older waiting runs stay with maintainers. If your PR does not
|
|
qualify, it gets the ``need workflow approval`` label and a maintainer approves the runs after
|
|
looking at the change. The pending ``Workflow approval`` status in the PR's checks says why the
|
|
runs wait. That can take a while, so please do not ping maintainers about it; it is not a judgement
|
|
of your PR. Once the runs are approved, by a maintainer or automatically for a later commit, the
|
|
label is removed and the status turns green.
|
|
|
|
Maintainers can see each decision, the CI jobs selective checks expect to run and their estimated
|
|
time in the summary of the ``Auto approve workflow runs`` workflow. The rules are described in
|
|
more detail in `CI workflows <../dev/breeze/doc/ci/05_workflows.md>`__.
|
|
|
|
Gen-AI Assisted contributions
|
|
-----------------------------
|
|
|
|
Generally, it's fine to use Gen-AI tools to help you create Pull Requests for Apache Airflow as long as you
|
|
adhere to the following guidelines:
|
|
|
|
* Follow the `Apache Software Foundation (ASF) Generative Tooling guidelines <https://www.apache.org/legal/generative-tooling.html>`__.
|
|
* Ensure that you review and understand all code generated by Gen-AI tools before including it in your PR -
|
|
do not blindly trust the generated code.
|
|
* Make sure that the generated code adheres to the project's coding standards and best practices described
|
|
above
|
|
* Make sure to run all relevant static checks and tests locally to verify that the generated code works as
|
|
intended and does not introduce any issues.
|
|
* State in your PR description that you have used Gen-AI tools to assist in creating the PR.
|
|
* Be prepared to explain and justify the use of Gen-AI tools if asked by project maintainers or reviewers.
|
|
* Remember that the final responsibility for the code in your PR lies with you, regardless of whether
|
|
it was generated by a tool or written by you.
|
|
* By contributing code generated by Gen-AI tools, you agree to comply with the project's licensing terms
|
|
and any applicable laws and regulations.
|
|
* Blindly copy & pasting code from Gen-AI tools is detrimental as it might introduce security and
|
|
stability risks to the project. Maintainers that spot such behaviours will have no choice but to
|
|
close the related PRs. It adds extra burden on the maintainers and doesn't help the project.
|
|
The contributor reputation is also impacted as maintainers will lose confidence and might block
|
|
the user from making further contributions.
|
|
* Review your code to make sure it does not contain unrelated changes. Often Gen-AI tools
|
|
might introduce changes that are not related to the problem you are trying to solve. Such
|
|
unrelated changes should be removed from the PR before it is submitted for review. Relying on
|
|
maintainers to spot such unrelated changes is unfair and adds extra burden on them.
|
|
|
|
When a contributor does not follow these guidelines, maintainers may close the PR (or all of
|
|
that contributor's open PRs) with a comment pointing to this section of the documentation, so
|
|
that maintainer review time is not spent on unvetted Gen-AI-generated code. Repeated disregard
|
|
of the Gen-AI guidelines is treated the same way as any other sustained disruptive behaviour
|
|
toward the project: it is handled under the project-wide
|
|
`Community escalation process <../COMMUNITY_ESCALATION.md>`_, which describes the further
|
|
steps that may follow (PMC-level blocking, ASF-Infrastructure-level blocking, reporting the
|
|
account to GitHub) and how a contributor can appeal any such decision by emailing the PMC at
|
|
``private@airflow.apache.org``.
|
|
|
|
Requirement to resolve all conversations
|
|
----------------------------------------
|
|
|
|
We have decided to enable the requirement to resolve all the open conversations in a
|
|
PR in order to make it merge-able. You will see in the status of the PR that it needs to have all the
|
|
conversations resolved before it can be merged.
|
|
|
|
The goal is to make it easier to see when there are some conversations that are not
|
|
resolved for everyone involved in the PR - author, reviewers and maintainers who try to figure out if
|
|
the PR is ready to merge and - eventually - merge it. The goal is also to use conversations more as a "soft" way
|
|
to request changes and limit the use of ``Request changes`` status to only those cases when the maintainer
|
|
is sure that the PR should not be merged in the current state. This leads to faster review/merge
|
|
cycle and less problems with stalled PRs that have ``Request changes`` status but all the issues are
|
|
already solved.
|
|
|
|
.. _coding_style:
|
|
|
|
Coding style and best practices
|
|
-------------------------------
|
|
|
|
Most of our coding style rules are enforced programmatically by ruff and mypy, which are run automatically
|
|
with static checks and on every Pull Request (PR), but there are some rules that are not yet automated and
|
|
are more Airflow specific or semantic than style.
|
|
|
|
Don't Use Asserts Outside Tests
|
|
...............................
|
|
|
|
Our community agreed that to various reasons we do not use ``assert`` in production code of Apache Airflow.
|
|
For details check the relevant `mailing list thread <https://lists.apache.org/thread.html/bcf2d23fcd79e21b3aac9f32914e1bf656e05ffbcb8aa282af497a2d%40%3Cdev.airflow.apache.org%3E>`_.
|
|
|
|
In other words instead of doing:
|
|
|
|
.. code-block:: python
|
|
|
|
assert some_predicate()
|
|
|
|
you should do:
|
|
|
|
.. code-block:: python
|
|
|
|
if not some_predicate():
|
|
handle_the_case()
|
|
|
|
The one exception to this is if you need to make an assert for type checking (which should be almost a last resort) you can do this:
|
|
|
|
.. code-block:: python
|
|
|
|
if TYPE_CHECKING:
|
|
assert isinstance(x, MyClass)
|
|
|
|
|
|
Database Session Handling
|
|
.........................
|
|
|
|
**Explicit is better than implicit.** If a function accepts a ``session`` parameter it should not commit the
|
|
transaction itself. Session management is up to the caller.
|
|
|
|
To make this easier, there is the ``create_session`` helper:
|
|
|
|
.. code-block:: python
|
|
|
|
from sqlalchemy.orm import Session
|
|
|
|
from airflow.utils.session import create_session
|
|
|
|
|
|
def my_call(x, y, *, session: Session):
|
|
...
|
|
# You MUST not commit the session here.
|
|
|
|
|
|
with create_session() as session:
|
|
my_call(x, y, session=session)
|
|
|
|
.. warning::
|
|
**DO NOT** add a default to the ``session`` argument **unless** ``@provide_session`` is used.
|
|
|
|
If this function is designed to be called by "end-users" (i.e. Dag authors) then using the ``@provide_session`` wrapper is okay:
|
|
|
|
.. code-block:: python
|
|
|
|
from sqlalchemy.orm import Session
|
|
|
|
from airflow.utils.session import NEW_SESSION, provide_session
|
|
|
|
|
|
@provide_session
|
|
def my_method(arg, *, session: Session = NEW_SESSION):
|
|
...
|
|
# You SHOULD not commit the session here. The wrapper will take care of commit()/rollback() if exception
|
|
|
|
In both cases, the ``session`` argument is a `keyword-only argument`_. This is the most preferred form if
|
|
possible, although there are some exceptions in the code base where this cannot be used, due to backward
|
|
compatibility considerations. In most cases, ``session`` argument should be last in the argument list.
|
|
|
|
.. _`keyword-only argument`: https://www.python.org/dev/peps/pep-3102/
|
|
|
|
|
|
Don't use time() for duration calculations
|
|
..........................................
|
|
|
|
If you wish to compute the time difference between two events with in the same process, use
|
|
``time.monotonic()``, not ``time.time()`` nor ``timezone.utcnow()``.
|
|
|
|
If you are measuring duration for performance reasons, then ``time.perf_counter()`` should be used. (On many
|
|
platforms, this uses the same underlying clock mechanism as monotonic, but ``perf_counter`` is guaranteed to be
|
|
the highest accuracy clock on the system, monotonic is simply "guaranteed" to not go backwards.)
|
|
|
|
If you wish to time how long a block of code takes, use ``Stats.timer()`` -- either with a metric name, which
|
|
will be timed and submitted automatically:
|
|
|
|
.. code-block:: python
|
|
|
|
from airflow.sdk.observability.stats import Stats
|
|
|
|
...
|
|
|
|
with Stats.timer("my_timer_metric"):
|
|
...
|
|
|
|
or to time but not send a metric:
|
|
|
|
.. code-block:: python
|
|
|
|
from airflow.sdk.observability.stats import Stats
|
|
|
|
...
|
|
|
|
with Stats.timer() as timer:
|
|
...
|
|
|
|
log.info("Code took %.3f ms", timer.duration)
|
|
|
|
For full docs on ``timer()`` check out `shared/observability/src/airflow_shared/observability/metrics/base_stats_logger.py`_.
|
|
|
|
If the start_date of a duration calculation needs to be stored in a database, then this has to be done using
|
|
datetime objects. In all other cases, using datetime for duration calculation MUST be avoided as creating and
|
|
diffing datetime operations are (comparatively) slow.
|
|
|
|
Templated fields in Operator's __init__ method
|
|
..............................................
|
|
|
|
Airflow Operators might have some fields added to the list of ``template_fields``. Such fields should be
|
|
set in the constructor (``__init__`` method) of the operator and usually their values should
|
|
come from the ``__init__`` method arguments. The reason for that is that the templated fields
|
|
are evaluated at the time of the operator execution and when you pass arguments to the operator
|
|
in the Dag, the fields that are set on the class just before the ``execute`` method is called
|
|
are processed through templating engine and the fields values are set to the result of applying the
|
|
templating engine to the fields (in case the field is a structure such as dict or list, the templating
|
|
engine is applied to all the values of the structure).
|
|
|
|
That's why we expect two things in case of ``template fields``:
|
|
|
|
* with a few exceptions, only self.field = field should be happening in the operator's constructor
|
|
* validation of the fields should be done in the ``execute`` method, not in the constructor because in
|
|
the constructor, the field value might be a templated value, not the final value.
|
|
|
|
The exceptions are cases where we want to assign empty default value to a mutable field (list or dict)
|
|
or when we have a more complex structure which we want to convert into a different format (say dict or list)
|
|
but where we want to keep the original strings in the converted structure.
|
|
|
|
In such cases we can usually do something like this
|
|
|
|
.. code-block:: python
|
|
|
|
def __init__(self, *, my_field: list[str] = None, **kwargs):
|
|
super().__init__(**kwargs)
|
|
my_field = my_field or []
|
|
self.my_field = my_field
|
|
|
|
For such a mutable default the ``if my_field is None: my_field = []`` spelling is equally fine.
|
|
|
|
Checks that only ask **whether an argument was passed** are the other exception, and they belong in
|
|
the constructor — it is the only place that can answer the question. With
|
|
``render_template_as_native_obj=True`` a field that *was* passed can render to
|
|
``None``, so in ``execute`` a supplied argument is indistinguishable from a missing one. Write the
|
|
check as ``is None`` / ``is not None``, never as a truthiness test — a supplied argument can itself
|
|
be ``""``, ``0`` or an empty list:
|
|
|
|
.. code-block:: python
|
|
|
|
from airflow.utils.helpers import exactly_one
|
|
|
|
|
|
def __init__(self, *, command: str | None = None, powershell: str | None = None, **kwargs):
|
|
if not exactly_one(command is not None, powershell is not None):
|
|
raise ValueError("Must provide exactly one of 'command' or 'powershell'")
|
|
super().__init__(**kwargs)
|
|
self.command = command
|
|
self.powershell = powershell
|
|
|
|
``self.field is None`` works the same way, after the assignment. Anything that looks at the *value*
|
|
still has to move to ``execute``, and the operand has to be the field itself — indirection such as
|
|
``all(v is None for v in [a, b])`` is still flagged.
|
|
|
|
Converting an existing truthiness check is not neutral: a guard that raises when two arguments are
|
|
*both* set only gets stricter, but ``if not field: raise`` gets looser — ``""`` and ``0`` start
|
|
passing, and that half is a value check. ``not exactly_one(a, b)`` carries both, since it also
|
|
requires at least one.
|
|
|
|
Operators that support ``start_from_trigger`` are the third exception. The scheduler sends the task
|
|
straight to the triggerer and ``execute`` never runs; the triggerer renders the ``trigger_kwargs``
|
|
entries whose key is both an operator template field and an attribute of the trigger. Copying such
|
|
a field verbatim into ``start_trigger_args`` under its own name is therefore fine in the
|
|
constructor:
|
|
|
|
.. code-block:: python
|
|
|
|
def __init__(self, *, job: dict, start_from_trigger: bool = False, **kwargs):
|
|
super().__init__(**kwargs)
|
|
self.job = job
|
|
self.start_from_trigger = start_from_trigger
|
|
if start_from_trigger:
|
|
self.start_trigger_args = dataclasses.replace(
|
|
self.start_trigger_args, trigger_kwargs={"job": self.job}
|
|
)
|
|
|
|
A transformed value, a copy under a key that is not a template field of the operator, and a template
|
|
field passed as ``timeout`` or ``next_kwargs`` are never rendered and are still flagged. A copy under
|
|
the name of a *different* template field is rendered, under that key, but is flagged as well — the
|
|
entry no longer holds the field its key names.
|
|
|
|
The hook only verifies the operator half of that pair: it cannot tell whether the key is also an
|
|
attribute of the trigger class, so a passing hook is not confirmation that the kwarg will be rendered.
|
|
|
|
The reason for doing it is that we are working on a cleaning up our code to have
|
|
`prek hook <../scripts/ci/prek/validate_operators_init.py>`_
|
|
that will make sure all the cases where logic (such as validation and complex conversion)
|
|
is not done in the constructor are detected in PRs.
|
|
|
|
Don't raise AirflowException directly
|
|
..............................................
|
|
|
|
Our community has decided to stop adding new ``raise AirflowException`` and to adopt the following practices when an exception is necessary. For details check the relevant `mailing list thread <https://lists.apache.org/thread/t8bnhyqy77kq4fk7fj3fmjd5wo9kv6w0>`_.
|
|
|
|
1. In most cases, we should prioritize using Python's standard exceptions (e.g., ``ValueError``, ``TypeError``, ``OSError``)
|
|
instead of wrapping everything in ``AirflowException``.
|
|
2. Within ``airflow-core``, we should define and utilize more specific exception classes under ``airflow-core/src/airflow/exceptions.py``.
|
|
3. For provider-specific implementations, exceptions should be defined within ``providers/<provider>/src/airflow/providers/<provider>/exceptions.py``.
|
|
|
|
The use of points 2 and 3 should only be considered when point 1 is inappropriate, which should be a rare occurrence.
|
|
|
|
In other words instead of doing:
|
|
|
|
.. code-block:: python
|
|
|
|
if key not in conf:
|
|
raise AirflowException(f"Required key {key} is missing")
|
|
|
|
you should do:
|
|
|
|
.. code-block:: python
|
|
|
|
if key not in conf:
|
|
raise ValueError(f"Required key {key} is missing")
|
|
|
|
-----------
|
|
|
|
If you want to learn what are the options for your development environment, follow to the
|
|
`Development environments <06_development_environments.rst>`_ document.
|