-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Add @now, @latest, @start_date, & @end_date exec date short-cuts to a… #1408
Conversation
|
@@ -607,6 +636,10 @@ class CLIFactory(object): | |||
'execution_date': Arg( | |||
("execution_date",), help="The execution date of the DAG", | |||
type=parsedate), | |||
'execution_date_enhanced': Arg( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not support this as part of execution_date
, rather than having two different params?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went that way first. At the point where the type converter (e.g. int, parsedate, etc...) is used from the argparse module, I don't have access to the dag. Hence, I need to execute these conversions later in code execution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove the type
argument in execution_date
and take charge. That would ensure consistent behavior of execution_date across the CLI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mistercrunch
I see execution date used in 4 CLI commands currently : render
, run
, test
, and task_state
. task_state
seems possible, but I'm not familiar with how run
is used internally. Also, what does render
do? I can possibly look at providing uniform handling if you think this feature makes sense for all of the commands above.
Coverage increased (+0.1%) to 67.172% when pulling 0670f16a1ebd06f9cc1882540cd60c045c3a2ca9 on r39132:master into c1f485f on airbnb:master. |
|
|
|
|
|
Coverage increased (+0.07%) to 67.098% when pulling 67c838890f70ad1a2d1307172c96ec46cbae1553 on r39132:master into c1f485f on airbnb:master. |
''' | ||
timestr = args.execution_date | ||
now = datetime.now() | ||
start_date = dag.default_args['start_date'] if \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think default args is always a dict, so maybe just start_date = dag.default_args.get('start_date')
? Same for end_date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought # 2 -- there are actually a few different "start dates"
dag.start_date
(could beNone
)dag.default_args['start_date']
(could beNone
or not even exist)dag.tasks[i].start_date
(could beNone
)
In the scheduler's logic, if a DAG has a start date, that takes precedence. If it doesn't, then min(t.start_date for t in dag.tasks)
is checked. Each task's start_date is either explicitly provided OR taken from default_args.
I'm not sure if that level of detail is required, but it occurs to me that default_args
might not be a robust place to look for the start date
Coverage increased (+0.07%) to 67.098% when pulling 09d94614c29811cb503d2cea14e9a616d1d05701 on r39132:master into c1f485f on airbnb:master. |
|
|
Coverage increased (+0.08%) to 67.105% when pulling 5a8251c43d66f68e93913fd2392314ecad297ef0 on r39132:master into c1f485f on airbnb:master. |
Coverage increased (+0.08%) to 67.105% when pulling 5a8251c43d66f68e93913fd2392314ecad297ef0 on r39132:master into c1f485f on airbnb:master. |
|
Coverage increased (+0.09%) to 67.121% when pulling f4c0b3abfc5233ec71854bdf4222148349b18173 on r39132:master into c1f485f on airbnb:master. |
@@ -336,6 +336,83 @@ def list_tasks(args, dag=None): | |||
print("\n".join(sorted(tasks))) | |||
|
|||
|
|||
def get_start_date(dag): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be a method or @Property of airflow.models.DAG
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good idea to centralize this logic, since it's also used by the scheduler. I think a private method like dag._get_start_date()
would be best since we already have a start_date
attribute that would be returned by the method unless start_date is None
.
|
Coverage decreased (-0.02%) to 67.129% when pulling 82fbb4eac2ed156cba7fc7db83eaaec0689585d9 on r39132:master into 12b542d on airbnb:master. |
|
Dear Airflow Maintainers,
Please accept the following PR that
airflow test
CLI #1398Reminder to contributors:
…irflow test