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

TimeSensorAsync breaks if target_time is timezone-aware #24736

Closed
1 of 2 tasks
Gollum999 opened this issue Jun 29, 2022 · 3 comments · Fixed by #25221
Closed
1 of 2 tasks

TimeSensorAsync breaks if target_time is timezone-aware #24736

Gollum999 opened this issue Jun 29, 2022 · 3 comments · Fixed by #25221
Labels
area:async-operators AIP-40: Deferrable ("Async") Operators kind:feature Feature Requests

Comments

@Gollum999
Copy link
Contributor

Apache Airflow version

2.3.2 (latest released)

What happened

TimeSensorAsync fails with the following error if target_time is aware:

[2022-06-29, 05:09:11 CDT] {taskinstance.py:1889} ERROR - Task failed with exception
Traceback (most recent call last):
  File "/opt/conda/envs/production/lib/python3.9/site-packages/airflow/sensors/time_sensor.py", line 60, in execute
    trigger=DateTimeTrigger(moment=self.target_datetime),
  File "/opt/conda/envs/production/lib/python3.9/site-packages/airflow/triggers/temporal.py", line 42, in __init__
    raise ValueError(f"The passed datetime must be using Pendulum's UTC, not {moment.tzinfo!r}")
ValueError: The passed datetime must be using Pendulum's UTC, not Timezone('America/Chicago')

What you think should happen instead

Given the fact that TimeSensor correctly handles timezones (#9882), this seems like a bug. TimeSensorAsync should be a drop-in replacement for TimeSensor, and therefore should have the same timezone behavior.

How to reproduce

#!/usr/bin/env python3
import datetime

from airflow.decorators import dag
from airflow.sensors.time_sensor import TimeSensor, TimeSensorAsync
import pendulum


@dag(
    start_date=datetime.datetime(2022, 6, 29),
    schedule_interval='@daily',
)
def time_sensor_dag():
    naive_time1 = datetime.time( 0,  1)
    aware_time1 = datetime.time( 0,  1).replace(tzinfo=pendulum.local_timezone())
    naive_time2 = pendulum.time(23, 59)
    aware_time2 = pendulum.time(23, 59).replace(tzinfo=pendulum.local_timezone())

    TimeSensor(task_id='naive_time1', target_time=naive_time1, mode='reschedule')
    TimeSensor(task_id='naive_time2', target_time=naive_time2, mode='reschedule')
    TimeSensor(task_id='aware_time1', target_time=aware_time1, mode='reschedule')
    TimeSensor(task_id='aware_time2', target_time=aware_time2, mode='reschedule')

    TimeSensorAsync(task_id='async_naive_time1', target_time=naive_time1)
    TimeSensorAsync(task_id='async_naive_time2', target_time=naive_time2)
    TimeSensorAsync(task_id='async_aware_time1', target_time=aware_time1)  # fails
    TimeSensorAsync(task_id='async_aware_time2', target_time=aware_time2)  # fails

dag = time_sensor_dag()

This can also happen if the target_time is naive and core.default_timezone = system.

Operating System

CentOS Stream 8

Versions of Apache Airflow Providers

N/A

Deployment

Other

Deployment details

Standalone

Anything else

No response

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@Gollum999 Gollum999 added area:core kind:bug This is a clearly a bug labels Jun 29, 2022
@uranusjr uranusjr added area:async-operators AIP-40: Deferrable ("Async") Operators and removed area:core labels Jun 30, 2022
@uranusjr
Copy link
Member

Perhaps we could allow all Pendulum timezones instead of requiring UTC. Not sure.

@uranusjr uranusjr added kind:feature Feature Requests and removed kind:bug This is a clearly a bug labels Jun 30, 2022
@uranusjr
Copy link
Member

Convert to a feature since not allowing non-UTC is an explicit design choice made in the current implementation. Need some context on this to decide what best to do here @andrewgodwin

@andrewgodwin
Copy link
Contributor

I don't see a problem with adding timezone conversion code to convert it to UTC at call time - I just add UTC-only checks to all timezone items by default if I didn't specifically write timezone support, so there's a nice clean error rather than weird behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:async-operators AIP-40: Deferrable ("Async") Operators kind:feature Feature Requests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants