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

Clarify pendulum use in timezone cases #21646

Merged
merged 3 commits into from Feb 18, 2022

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Feb 17, 2022

It is important to use Pendulum in case timezone is used - because
there are a number of limitations coming from using stdlib
timezone implementation.

However our documentation was not very clear about it, especially
some examples shown using standard datetime in DAGs which could
mislead our users to continue using datetime if they use timezone.

This PR clarifies and stresses the use of pendulum is necessary
when timezone is used. Also it points to the documentation
in case serialization throws error about not using Pendulum
so that the users can learn about the reasoning.

See also #20070


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes provider related issues area:lineage area:Scheduler Scheduler or dag parsing Issues area:serialization kind:documentation labels Feb 17, 2022
@potiuk potiuk requested a review from uranusjr February 17, 2022 16:03
@potiuk potiuk added this to the Airflow 2.2.4 milestone Feb 17, 2022
@potiuk potiuk added the type:doc-only Changelog: Doc Only label Feb 17, 2022
docs/apache-airflow/faq.rst Outdated Show resolved Hide resolved
Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

nits - lgtm

@josh-fell
Copy link
Contributor

josh-fell commented Feb 17, 2022

... some examples shown using standard datetime in DAGs which could mislead our users to continue using datetime if they use timezone

Should all of the example DAGs get updated as well? This feels like a similar preemptive move we did to prevent user headaches by adding catchup=False throughout.

docs/apache-airflow/timezone.rst Outdated Show resolved Hide resolved
@josh-fell
Copy link
Contributor

I think we are using "timezone" more frequently than "time zone". Just a little mixing of the two in places.

@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Feb 17, 2022
@potiuk
Copy link
Member Author

potiuk commented Feb 17, 2022

Should all of the example DAGs get updated as well? This feels like a similar preemptive move we did to prevent user headaches by adding catchup=False throughout.

That's a good idea. The examples are not "wrong" per se but indeed converting them to use pendulum might prevent users from adding non-pendulum timezones and lead them in the right direction.

@potiuk
Copy link
Member Author

potiuk commented Feb 17, 2022

I rebased/Updated all example dags (non-providers for now) and rst examples to explicitly mention pendulum.datetime + timezone and datetime.timedelta.

This I think is the best way to educate our users on using timezone.

UPDATING.md Outdated Show resolved Hide resolved
It is important to use Pendulum in case timezone is used - because
there are a number of limitations coming from using stdlib
timezone implementation.

However our documentation was not very clear about it, especially
some examples shown using standard datetime in DAGs which could
mislead our users to continue using datetime if they use timezone.

This PR clarifies and stresses the use of pendulum is necessary
when timezone is used. Also it points to the documentation
in case serialization throws error about not using Pendulum
so that the users can learn about the reasoning.

This is the first part of the change - the follow up will be
changing all provider examples to also use timezone and
pendulum explicitely.

See also apache#20070
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@potiuk
Copy link
Member Author

potiuk commented Feb 17, 2022

Ah... Hawkeye :)

@jedcunningham jedcunningham merged commit f011da2 into apache:main Feb 18, 2022
jedcunningham pushed a commit that referenced this pull request Feb 18, 2022
It is important to use Pendulum in case timezone is used - because
there are a number of limitations coming from using stdlib
timezone implementation.

However our documentation was not very clear about it, especially
some examples shown using standard datetime in DAGs which could
mislead our users to continue using datetime if they use timezone.

This PR clarifies and stresses the use of pendulum is necessary
when timezone is used. Also it points to the documentation
in case serialization throws error about not using Pendulum
so that the users can learn about the reasoning.

This is the first part of the change - the follow up will be
changing all provider examples to also use timezone and
pendulum explicitly.

See also #20070

(cherry picked from commit f011da2)
hammerhead added a commit to crate/cratedb-airflow-tutorial that referenced this pull request Apr 5, 2022
This aligns our DAGs with the Airflow example DAGs. See apache/airflow#21646.
marijaselakovic pushed a commit to crate/cratedb-airflow-tutorial that referenced this pull request Apr 5, 2022
* Use latest actions

* Use pendulum instead of datetime for a DAG's start_date

This aligns our DAGs with the Airflow example DAGs. See apache/airflow#21646.

* Update GitHub Actions with dependabot

* Remove outdated file, directory doesn't exist any more

* Add include directory to pylint

* Update Airflow to 2.2.5
@potiuk potiuk deleted the add-docs-about-timezone branch July 29, 2022 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:lineage area:Scheduler Scheduler or dag parsing Issues area:serialization full tests needed We need to run full set of tests for this PR to merge kind:documentation provider:cncf-kubernetes Kubernetes provider related issues type:doc-only Changelog: Doc Only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants