You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/07/14 05:26:23 UTC
[GitHub] [airflow] uranusjr opened a new pull request, #25045: Unify summary/description logic to timetable
uranusjr opened a new pull request, #25045:
URL: https://github.com/apache/airflow/pull/25045
These values are pulled directly from the timetable, so we can unify the dataset case into it instead of needing extra if-else cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #25045: Unify summary/description logic to timetable
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25045:
URL: https://github.com/apache/airflow/pull/25045#discussion_r921284396
##########
airflow/models/dag.py:
##########
@@ -2498,12 +2498,8 @@ def bulk_write_to_db(cls, dags: Collection["DAG"], session=NEW_SESSION):
orm_dag.max_active_tasks = dag.max_active_tasks
orm_dag.max_active_runs = dag.max_active_runs
orm_dag.has_task_concurrency_limits = any(t.max_active_tis_per_dag is not None for t in dag.tasks)
- if dag.schedule_on:
- orm_dag.schedule_interval = 'Dataset'
- orm_dag.timetable_description = 'Triggered by datasets.'
- else:
- orm_dag.schedule_interval = dag.schedule_interval
- orm_dag.timetable_description = dag.timetable.description
+ orm_dag.schedule_interval = dag.schedule_interval
Review Comment:
i tried making it dag.timetable.summary --- which would be probably the "right" way but then tests started failing so i aborted mission
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #25045: Unify summary/description logic to timetable
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25045:
URL: https://github.com/apache/airflow/pull/25045#discussion_r921284396
##########
airflow/models/dag.py:
##########
@@ -2498,12 +2498,8 @@ def bulk_write_to_db(cls, dags: Collection["DAG"], session=NEW_SESSION):
orm_dag.max_active_tasks = dag.max_active_tasks
orm_dag.max_active_runs = dag.max_active_runs
orm_dag.has_task_concurrency_limits = any(t.max_active_tis_per_dag is not None for t in dag.tasks)
- if dag.schedule_on:
- orm_dag.schedule_interval = 'Dataset'
- orm_dag.timetable_description = 'Triggered by datasets.'
- else:
- orm_dag.schedule_interval = dag.schedule_interval
- orm_dag.timetable_description = dag.timetable.description
+ orm_dag.schedule_interval = dag.schedule_interval
Review Comment:
yesterday i tried doing the same thing, but making it dag.timetable.summary --- which would be probably the "right" way -- but then tests started failing so i aborted mission
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #25045: Unify summary/description logic to timetable
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25045:
URL: https://github.com/apache/airflow/pull/25045#discussion_r921397899
##########
airflow/models/dag.py:
##########
@@ -2498,12 +2498,8 @@ def bulk_write_to_db(cls, dags: Collection["DAG"], session=NEW_SESSION):
orm_dag.max_active_tasks = dag.max_active_tasks
orm_dag.max_active_runs = dag.max_active_runs
orm_dag.has_task_concurrency_limits = any(t.max_active_tis_per_dag is not None for t in dag.tasks)
- if dag.schedule_on:
- orm_dag.schedule_interval = 'Dataset'
- orm_dag.timetable_description = 'Triggered by datasets.'
- else:
- orm_dag.schedule_interval = dag.schedule_interval
- orm_dag.timetable_description = dag.timetable.description
+ orm_dag.schedule_interval = dag.schedule_interval
Review Comment:
ok so ... here's what's going on... i didn't see that you set DAG.schedule_interval = 'Dataset' in `DAG.__init__`
so i assumed it would be None here.
the way i tried this before, I had `DAG.schedule_interval == None` and `DagModel.schudule_interval == 'Dataset'`, with the thought that `DAG.schedule_interval` _should be_ `None`, while `DagModel.schedule_interval` would be better named `DagModel.schedule_label` or something.
the idea is that there are two different things going on here. one is, the "label" presented under "schedule" column in dags page. other is, the `schedule_interval` param for DAG object -- which for datasets is None -- cus there is _no_ schedule interval for this kind of dag. it may best to make clear this distinction and leave DAG.schedule_interval `None` while setting DagModel.schedule_interval to `Dataset` (and perhaps renaming DagModel.schedule_interval to DagModel.schedule_summary or DagModel.schedule_label). But doing this runs afoul of some tests and will be more work, and we need not let perfect stand in the way of good, so i'll approve.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr merged pull request #25045: Unify summary/description logic to timetable
Posted by GitBox <gi...@apache.org>.
uranusjr merged PR #25045:
URL: https://github.com/apache/airflow/pull/25045
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #25045: Unify summary/description logic to timetable
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25045:
URL: https://github.com/apache/airflow/pull/25045#discussion_r921388987
##########
airflow/models/dag.py:
##########
@@ -2498,12 +2498,8 @@ def bulk_write_to_db(cls, dags: Collection["DAG"], session=NEW_SESSION):
orm_dag.max_active_tasks = dag.max_active_tasks
orm_dag.max_active_runs = dag.max_active_runs
orm_dag.has_task_concurrency_limits = any(t.max_active_tis_per_dag is not None for t in dag.tasks)
- if dag.schedule_on:
- orm_dag.schedule_interval = 'Dataset'
- orm_dag.timetable_description = 'Triggered by datasets.'
- else:
- orm_dag.schedule_interval = dag.schedule_interval
- orm_dag.timetable_description = dag.timetable.description
+ orm_dag.schedule_interval = dag.schedule_interval
Review Comment:
oh -- maybe i have that wrong! lemme look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #25045: Unify summary/description logic to timetable
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25045:
URL: https://github.com/apache/airflow/pull/25045#discussion_r921809918
##########
airflow/models/dag.py:
##########
@@ -2498,12 +2498,8 @@ def bulk_write_to_db(cls, dags: Collection["DAG"], session=NEW_SESSION):
orm_dag.max_active_tasks = dag.max_active_tasks
orm_dag.max_active_runs = dag.max_active_runs
orm_dag.has_task_concurrency_limits = any(t.max_active_tis_per_dag is not None for t in dag.tasks)
- if dag.schedule_on:
- orm_dag.schedule_interval = 'Dataset'
- orm_dag.timetable_description = 'Triggered by datasets.'
- else:
- orm_dag.schedule_interval = dag.schedule_interval
- orm_dag.timetable_description = dag.timetable.description
+ orm_dag.schedule_interval = dag.schedule_interval
Review Comment:
> while DagModel.schedule_interval does not have meaning and is just a label, DAG.schedule_interval does
er, at least i think it does, doesn't it? like that's where you set `@daily` or `30 7 * * *`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #25045: Unify summary/description logic to timetable
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25045:
URL: https://github.com/apache/airflow/pull/25045#discussion_r921283412
##########
airflow/models/dag.py:
##########
@@ -2498,12 +2498,8 @@ def bulk_write_to_db(cls, dags: Collection["DAG"], session=NEW_SESSION):
orm_dag.max_active_tasks = dag.max_active_tasks
orm_dag.max_active_runs = dag.max_active_runs
orm_dag.has_task_concurrency_limits = any(t.max_active_tis_per_dag is not None for t in dag.tasks)
- if dag.schedule_on:
- orm_dag.schedule_interval = 'Dataset'
- orm_dag.timetable_description = 'Triggered by datasets.'
- else:
- orm_dag.schedule_interval = dag.schedule_interval
- orm_dag.timetable_description = dag.timetable.description
+ orm_dag.schedule_interval = dag.schedule_interval
Review Comment:
@uranusjr this needs to be 'Dataset' but right now it will be `None`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #25045: Unify summary/description logic to timetable
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25045:
URL: https://github.com/apache/airflow/pull/25045#discussion_r921397899
##########
airflow/models/dag.py:
##########
@@ -2498,12 +2498,8 @@ def bulk_write_to_db(cls, dags: Collection["DAG"], session=NEW_SESSION):
orm_dag.max_active_tasks = dag.max_active_tasks
orm_dag.max_active_runs = dag.max_active_runs
orm_dag.has_task_concurrency_limits = any(t.max_active_tis_per_dag is not None for t in dag.tasks)
- if dag.schedule_on:
- orm_dag.schedule_interval = 'Dataset'
- orm_dag.timetable_description = 'Triggered by datasets.'
- else:
- orm_dag.schedule_interval = dag.schedule_interval
- orm_dag.timetable_description = dag.timetable.description
+ orm_dag.schedule_interval = dag.schedule_interval
Review Comment:
ok so ... here's what's going on... i didn't see that you set DAG.schedule_interval = 'Dataset' in DAG.__init__
so i assumed it would be None here.
the way i tried this before, I had DAG.schedule_interval == None and DagModel.schudule_interval == 'Dataset', with the thought that DAG.schedule_interval should be None, while DagModel.schedule_interval would be better named DagModel.schedule_label or something.
the idea is that there are two different things going on here. one is, the "label" presented under "schedule" column in dags page. other is, the schedule_interval param for DAG object -- which for datasets is None -- cus there is no schedule interval for this kind of dag. it may best to make clear this distinction and leave DAG.schedule_interval None while setting DagModel.schedule_interval to Dataset (and perhaps renaming DagModel.schedule_interval to DagModel.schedule_summary or DagModel.schedule_label). But doing this runs afoul of some tests and will be more work, and we need not let perfect stand in the way of good, so i'll approve.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #25045: Unify summary/description logic to timetable
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25045:
URL: https://github.com/apache/airflow/pull/25045#discussion_r921758228
##########
airflow/models/dag.py:
##########
@@ -2498,12 +2498,8 @@ def bulk_write_to_db(cls, dags: Collection["DAG"], session=NEW_SESSION):
orm_dag.max_active_tasks = dag.max_active_tasks
orm_dag.max_active_runs = dag.max_active_runs
orm_dag.has_task_concurrency_limits = any(t.max_active_tis_per_dag is not None for t in dag.tasks)
- if dag.schedule_on:
- orm_dag.schedule_interval = 'Dataset'
- orm_dag.timetable_description = 'Triggered by datasets.'
- else:
- orm_dag.schedule_interval = dag.schedule_interval
- orm_dag.timetable_description = dag.timetable.description
+ orm_dag.schedule_interval = dag.schedule_interval
Review Comment:
Yes the key “problem” per-se is that `DAG.schedule_interval` at this point does not actually have any functionality beyond determining the label name we show on the UI. A rename here is really needed to reflect that, and this change makes sense if you look pass the attribute name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #25045: Unify summary/description logic to timetable
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25045:
URL: https://github.com/apache/airflow/pull/25045#discussion_r921806232
##########
airflow/models/dag.py:
##########
@@ -2498,12 +2498,8 @@ def bulk_write_to_db(cls, dags: Collection["DAG"], session=NEW_SESSION):
orm_dag.max_active_tasks = dag.max_active_tasks
orm_dag.max_active_runs = dag.max_active_runs
orm_dag.has_task_concurrency_limits = any(t.max_active_tis_per_dag is not None for t in dag.tasks)
- if dag.schedule_on:
- orm_dag.schedule_interval = 'Dataset'
- orm_dag.timetable_description = 'Triggered by datasets.'
- else:
- orm_dag.schedule_interval = dag.schedule_interval
- orm_dag.timetable_description = dag.timetable.description
+ orm_dag.schedule_interval = dag.schedule_interval
Review Comment:
yeah i guess what i am saying though is that while DagModel.schedule_interval does not have meaning and is just a label, DAG.schedule_interval _does_, and so that's why it feels a little off to store the label `Dataset` on `DAG.schedule_interval`. storing on the DagModel attr makes sense but, in a perfect world, we wouldn't also set it on DagModel. anyway, nothing really to worry about.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #25045: Unify summary/description logic to timetable
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25045:
URL: https://github.com/apache/airflow/pull/25045#discussion_r921343259
##########
airflow/models/dag.py:
##########
@@ -2498,12 +2498,8 @@ def bulk_write_to_db(cls, dags: Collection["DAG"], session=NEW_SESSION):
orm_dag.max_active_tasks = dag.max_active_tasks
orm_dag.max_active_runs = dag.max_active_runs
orm_dag.has_task_concurrency_limits = any(t.max_active_tis_per_dag is not None for t in dag.tasks)
- if dag.schedule_on:
- orm_dag.schedule_interval = 'Dataset'
- orm_dag.timetable_description = 'Triggered by datasets.'
- else:
- orm_dag.schedule_interval = dag.schedule_interval
- orm_dag.timetable_description = dag.timetable.description
+ orm_dag.schedule_interval = dag.schedule_interval
Review Comment:
Hmm those two should be the same value (due to the assignment in `__init__` so something somewhere is changing the value and we need to find out what that is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #25045: Unify summary/description logic to timetable
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25045:
URL: https://github.com/apache/airflow/pull/25045#discussion_r921397899
##########
airflow/models/dag.py:
##########
@@ -2498,12 +2498,8 @@ def bulk_write_to_db(cls, dags: Collection["DAG"], session=NEW_SESSION):
orm_dag.max_active_tasks = dag.max_active_tasks
orm_dag.max_active_runs = dag.max_active_runs
orm_dag.has_task_concurrency_limits = any(t.max_active_tis_per_dag is not None for t in dag.tasks)
- if dag.schedule_on:
- orm_dag.schedule_interval = 'Dataset'
- orm_dag.timetable_description = 'Triggered by datasets.'
- else:
- orm_dag.schedule_interval = dag.schedule_interval
- orm_dag.timetable_description = dag.timetable.description
+ orm_dag.schedule_interval = dag.schedule_interval
Review Comment:
ok so ... here's what's going on... i didn't see that you set DAG.schedule_interval = 'Dataset' in `DAG.__init__`
so i assumed it would be None here.
the way i tried this before, I had `DAG.schedule_interval == None` but `DagModel.schudule_interval == 'Dataset'`, with the thought that `DAG.schedule_interval` _should be_ `None`, while `DagModel.schedule_interval` would be better named `DagModel.schedule_label` or something.
the idea is that there are two different things going on here. one is, the "label" presented under "schedule" column in dags page. other is, the `schedule_interval` param for DAG object -- which for datasets is None -- cus there is _no_ schedule interval for this kind of dag. it may best to make clear this distinction and leave DAG.schedule_interval `None` while setting DagModel.schedule_interval to `Dataset` (and perhaps renaming DagModel.schedule_interval to DagModel.schedule_summary or DagModel.schedule_label). But _that_ will require a lot more work, and we need not let perfect stand in the way of good, so i'll approve.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #25045: Unify summary/description logic to timetable
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25045:
URL: https://github.com/apache/airflow/pull/25045#discussion_r921397899
##########
airflow/models/dag.py:
##########
@@ -2498,12 +2498,8 @@ def bulk_write_to_db(cls, dags: Collection["DAG"], session=NEW_SESSION):
orm_dag.max_active_tasks = dag.max_active_tasks
orm_dag.max_active_runs = dag.max_active_runs
orm_dag.has_task_concurrency_limits = any(t.max_active_tis_per_dag is not None for t in dag.tasks)
- if dag.schedule_on:
- orm_dag.schedule_interval = 'Dataset'
- orm_dag.timetable_description = 'Triggered by datasets.'
- else:
- orm_dag.schedule_interval = dag.schedule_interval
- orm_dag.timetable_description = dag.timetable.description
+ orm_dag.schedule_interval = dag.schedule_interval
Review Comment:
ok so ... here's what's going on... i didn't see that you set DAG.schedule_interval = 'Dataset' in `DAG.__init__`
so i assumed it would be None here.
the way i tried this before, I had `DAG.schedule_interval == None` and `DagModel.schudule_interval == 'Dataset'`, with the thought that `DAG.schedule_interval` _should be_ `None`, while `DagModel.schedule_interval` would be better named `DagModel.schedule_label` or something.
the idea is that there are two different things going on here. one is, the "label" presented under "schedule" column in dags page. other is, the `schedule_interval` param for DAG object -- which for datasets is None -- cus there is _no_ schedule interval for this kind of dag. it may best to make clear this distinction and leave DAG.schedule_interval `None` while setting DagModel.schedule_interval to `Dataset` (and perhaps renaming DagModel.schedule_interval to DagModel.schedule_summary or DagModel.schedule_label). But _that_ will require a lot more work, and we need not let perfect stand in the way of good, so i'll approve.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #25045: Unify summary/description logic to timetable
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25045:
URL: https://github.com/apache/airflow/pull/25045#discussion_r921397899
##########
airflow/models/dag.py:
##########
@@ -2498,12 +2498,8 @@ def bulk_write_to_db(cls, dags: Collection["DAG"], session=NEW_SESSION):
orm_dag.max_active_tasks = dag.max_active_tasks
orm_dag.max_active_runs = dag.max_active_runs
orm_dag.has_task_concurrency_limits = any(t.max_active_tis_per_dag is not None for t in dag.tasks)
- if dag.schedule_on:
- orm_dag.schedule_interval = 'Dataset'
- orm_dag.timetable_description = 'Triggered by datasets.'
- else:
- orm_dag.schedule_interval = dag.schedule_interval
- orm_dag.timetable_description = dag.timetable.description
+ orm_dag.schedule_interval = dag.schedule_interval
Review Comment:
ok so ... here's what's going on... i didn't see that you set DAG.schedule_interval = 'Dataset' in `DAG.__init__`
so i assumed it would be None here.
the way i did it, I had `DAG.schedule_interval == None` but `DagModel.schudule_interval == 'Dataset'`, with the thought that `DAG.schedule_interval` _should be_ `None`, while `DagModel.schedule_interval` would be better named `DagModel.schedule_label` or something.
the idea is that there are two different things going on here. one is, the "label" presented under "schedule" column in dags page. other is, the `schedule_interval` param for DAG object -- which for datasets is None -- cus there is _no_ schedule interval for this kind of dag. it may best to make clear this distinction and leave DAG.schedule_interval `None` while setting DagModel.schedule_interval to `Dataset` (and perhaps renaming DagModel.schedule_interval to DagModel.schedule_summary or DagModel.schedule_label). But _that_ will require a lot more work, we need not let perfect stand in the way of good, so i'll approve.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #25045: Unify summary/description logic to timetable
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25045:
URL: https://github.com/apache/airflow/pull/25045#discussion_r921397899
##########
airflow/models/dag.py:
##########
@@ -2498,12 +2498,8 @@ def bulk_write_to_db(cls, dags: Collection["DAG"], session=NEW_SESSION):
orm_dag.max_active_tasks = dag.max_active_tasks
orm_dag.max_active_runs = dag.max_active_runs
orm_dag.has_task_concurrency_limits = any(t.max_active_tis_per_dag is not None for t in dag.tasks)
- if dag.schedule_on:
- orm_dag.schedule_interval = 'Dataset'
- orm_dag.timetable_description = 'Triggered by datasets.'
- else:
- orm_dag.schedule_interval = dag.schedule_interval
- orm_dag.timetable_description = dag.timetable.description
+ orm_dag.schedule_interval = dag.schedule_interval
Review Comment:
ok so ... here's what's going on... i didn't see that you set DAG.schedule_interval = 'Dataset' in `DAG.__init__`
so i assumed it would be None here.
the way i did it, I had `DAG.schedule_interval == None` but `DagModel.schudule_interval == 'Dataset'`, with the thought that `DAG.schedule_interval` _should be_ `None`, while `DagModel.schedule_interval` would be better named `DagModel.schedule_label` or something.
the idea is that there are two different things going on here. one is, the "label" presented under "schedule" column in dags page. other is, the `schedule_interval` param for DAG object -- which for datasets is None -- cus there is _no_ schedule interval for this kind of dag. it may best to make clear this distinction and leave DAG.schedule_interval `None` while setting DagModel.schedule_interval to `Dataset` (and perhaps renaming DagModel.schedule_interval to DagModel.schedule_summary or DagModel.schedule_label). But _that_ will require a lot more work, and we need not let perfect stand in the way of good, so i'll approve.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org