You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@airflow.apache.org by Bas Harenslak <ba...@godatadriven.com> on 2019/06/22 18:49:54 UTC
AIP-22: Group ORM models by their logical usage instead of type
Hi all,
I’ve created AIP-22 and propose to move every ORM class to a module where one would logically expect them to be, instead of grouping all ORM models together, e.g. BaseOperator in airflow.operators (this is actually not an ORM class).
To me the benefit would be a more logically structured code base, with classes stored where you’d initially expect them to be, instead of having to know upfront if they’re ORM classes or not.
I’ve explained the plan in more detail in the AIP: https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-22%3A+Group+ORM+models+by+their+logical+usage+instead+of+type<https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-22:+Group+ORM+models+by+their+logical+usage+instead+of+type> and wonder if others think the same about it.
Needless to say, this would be a breaking change and only possible in Airflow 2.0.
Cheers,
Bas
Re: AIP-22: Group ORM models by their logical usage instead of type
Posted by Jarek Potiuk <Ja...@polidea.com>.
Hard to say as we have no stats but even in our very own airflow examples
we have plenty of "from airflow.models import DAG":
grep -R 'from airflow.models import DAG' .
./example_latest_only.py:from airflow.models import DAG
./example_python_operator.py:from airflow.models import DAG
./test_utils.py:from airflow.models import DAG
./example_bash_operator.py:from airflow.models import DAG
./example_short_circuit_operator.py:from airflow.models import DAG
./example_branch_operator.py:from airflow.models import DAG
./example_pig_operator.py:from airflow.models import DAG
./example_latest_only_with_trigger.py:from airflow.models import DAG
./subdags/subdag.py:from airflow.models import DAG
./example_skip_dag.py:from airflow.models import DAG
./example_trigger_target_dag.py:from airflow.models import DAG
./example_branch_python_dop_operator_3.py:from airflow.models import DAG
./example_subdag_operator.py:from airflow.models import DAG
grep -R 'from airflow import DAG' .
./example_docker_operator.py:from airflow import DAG
./tutorial.py:from airflow import DAG
./docker_copy_data.py:# from airflow import DAG
./example_passing_params_via_test_command.py:from airflow import DAG
./example_xcom.py:from airflow import DAG
./example_http_operator.py:from airflow import DAG
./example_trigger_controller_dag.py:from airflow import DAG
On Mon, Jun 24, 2019 at 8:43 AM Ash Berlin-Taylor <as...@apache.org> wrote:
> Don't most people do `from airflow import DAG` (i.e. they don't import it
> from models but airflow)?
>
> On 24 June 2019 00:07:49 BST, Jarek Potiuk <Ja...@polidea.com>
> wrote:
> >Just few comments to spark a discussion about backwards compatibility
> >for
> >this proposal:
> >
> >- Airflow uses Alembic to generate automated migration scripts. Not
> >sure
> >where it takes all the models from to generate the scripts (__init__ in
> >airflow.models maybe, or simply by scanning all classes). I do not
> >expect
> >any serious problems as alembic uses column/table names in its
> >migration
> >scripts but it would be worth checking if there are any negative
> >consequences
> >- I think it might make sense to keep airflow.models.__init__ in place
> >anyway to keep backwards compatibility maybe ?
> >- I think there is one particular case which is super-sensitive for
> >backwards compatibility models.DAG - which is used pretty much in all
> >DAGs
> >written by anyone (`from airflow.models import DAG` or `with
> >models.DAG(...` ). I think it's one thing to break some backwards
> >compatibility in edge cases and it's quite a different thing if pretty
> >much
> >all custom-written DAGs have to be corrected. I think this is yet
> >another
> >argument to keep at least the old __init__ and import the classes from
> >their new locations still keeping the old 'models' package - even if we
> >decide to move them. And then it might turn out the change will be
> >fully
> >backwards-compatible.
> >
> >J.
> >
> >On Sat, Jun 22, 2019 at 8:50 PM Bas Harenslak
> ><ba...@godatadriven.com>
> >wrote:
> >
> >> Hi all,
> >>
> >> I’ve created AIP-22 and propose to move every ORM class to a module
> >where
> >> one would logically expect them to be, instead of grouping all ORM
> >models
> >> together, e.g. BaseOperator in airflow.operators (this is actually
> >not an
> >> ORM class).
> >>
> >> To me the benefit would be a more logically structured code base,
> >with
> >> classes stored where you’d initially expect them to be, instead of
> >having
> >> to know upfront if they’re ORM classes or not.
> >>
> >> I’ve explained the plan in more detail in the AIP:
> >>
> >
> https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-22%3A+Group+ORM+models+by+their+logical+usage+instead+of+type
> >> <
> >>
> >
> https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-22:+Group+ORM+models+by+their+logical+usage+instead+of+type
> >
> >> and wonder if others think the same about it.
> >>
> >> Needless to say, this would be a breaking change and only possible in
> >> Airflow 2.0.
> >>
> >> Cheers,
> >> Bas
> >>
> >
> >
> >--
> >
> >Jarek Potiuk
> >Polidea <https://www.polidea.com/> | Principal Software Engineer
> >
> >M: +48 660 796 129 <+48660796129>
> >[image: Polidea] <https://www.polidea.com/>
>
--
Jarek Potiuk
Polidea <https://www.polidea.com/> | Principal Software Engineer
M: +48 660 796 129 <+48660796129>
[image: Polidea] <https://www.polidea.com/>
Re: AIP-22: Group ORM models by their logical usage instead of type
Posted by Ash Berlin-Taylor <as...@apache.org>.
Don't most people do `from airflow import DAG` (i.e. they don't import it from models but airflow)?
On 24 June 2019 00:07:49 BST, Jarek Potiuk <Ja...@polidea.com> wrote:
>Just few comments to spark a discussion about backwards compatibility
>for
>this proposal:
>
>- Airflow uses Alembic to generate automated migration scripts. Not
>sure
>where it takes all the models from to generate the scripts (__init__ in
>airflow.models maybe, or simply by scanning all classes). I do not
>expect
>any serious problems as alembic uses column/table names in its
>migration
>scripts but it would be worth checking if there are any negative
>consequences
>- I think it might make sense to keep airflow.models.__init__ in place
>anyway to keep backwards compatibility maybe ?
>- I think there is one particular case which is super-sensitive for
>backwards compatibility models.DAG - which is used pretty much in all
>DAGs
>written by anyone (`from airflow.models import DAG` or `with
>models.DAG(...` ). I think it's one thing to break some backwards
>compatibility in edge cases and it's quite a different thing if pretty
>much
>all custom-written DAGs have to be corrected. I think this is yet
>another
>argument to keep at least the old __init__ and import the classes from
>their new locations still keeping the old 'models' package - even if we
>decide to move them. And then it might turn out the change will be
>fully
>backwards-compatible.
>
>J.
>
>On Sat, Jun 22, 2019 at 8:50 PM Bas Harenslak
><ba...@godatadriven.com>
>wrote:
>
>> Hi all,
>>
>> I’ve created AIP-22 and propose to move every ORM class to a module
>where
>> one would logically expect them to be, instead of grouping all ORM
>models
>> together, e.g. BaseOperator in airflow.operators (this is actually
>not an
>> ORM class).
>>
>> To me the benefit would be a more logically structured code base,
>with
>> classes stored where you’d initially expect them to be, instead of
>having
>> to know upfront if they’re ORM classes or not.
>>
>> I’ve explained the plan in more detail in the AIP:
>>
>https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-22%3A+Group+ORM+models+by+their+logical+usage+instead+of+type
>> <
>>
>https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-22:+Group+ORM+models+by+their+logical+usage+instead+of+type>
>> and wonder if others think the same about it.
>>
>> Needless to say, this would be a breaking change and only possible in
>> Airflow 2.0.
>>
>> Cheers,
>> Bas
>>
>
>
>--
>
>Jarek Potiuk
>Polidea <https://www.polidea.com/> | Principal Software Engineer
>
>M: +48 660 796 129 <+48660796129>
>[image: Polidea] <https://www.polidea.com/>
Re: AIP-22: Group ORM models by their logical usage instead of type
Posted by Bas Harenslak <ba...@godatadriven.com>.
Anything inheriting from airflow.models.base.Base is found by Alembic, regardless the location of the script.
Bas
> On 24 Jun 2019, at 01:07, Jarek Potiuk <Ja...@polidea.com> wrote:
>
> Just few comments to spark a discussion about backwards compatibility for
> this proposal:
>
> - Airflow uses Alembic to generate automated migration scripts. Not sure
> where it takes all the models from to generate the scripts (__init__ in
> airflow.models maybe, or simply by scanning all classes). I do not expect
> any serious problems as alembic uses column/table names in its migration
> scripts but it would be worth checking if there are any negative
> consequences
> - I think it might make sense to keep airflow.models.__init__ in place
> anyway to keep backwards compatibility maybe ?
> - I think there is one particular case which is super-sensitive for
> backwards compatibility models.DAG - which is used pretty much in all DAGs
> written by anyone (`from airflow.models import DAG` or `with
> models.DAG(...` ). I think it's one thing to break some backwards
> compatibility in edge cases and it's quite a different thing if pretty much
> all custom-written DAGs have to be corrected. I think this is yet another
> argument to keep at least the old __init__ and import the classes from
> their new locations still keeping the old 'models' package - even if we
> decide to move them. And then it might turn out the change will be fully
> backwards-compatible.
>
> J.
>
> On Sat, Jun 22, 2019 at 8:50 PM Bas Harenslak <ba...@godatadriven.com>
> wrote:
>
>> Hi all,
>>
>> I’ve created AIP-22 and propose to move every ORM class to a module where
>> one would logically expect them to be, instead of grouping all ORM models
>> together, e.g. BaseOperator in airflow.operators (this is actually not an
>> ORM class).
>>
>> To me the benefit would be a more logically structured code base, with
>> classes stored where you’d initially expect them to be, instead of having
>> to know upfront if they’re ORM classes or not.
>>
>> I’ve explained the plan in more detail in the AIP:
>> https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-22%3A+Group+ORM+models+by+their+logical+usage+instead+of+type
>> <
>> https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-22:+Group+ORM+models+by+their+logical+usage+instead+of+type>
>> and wonder if others think the same about it.
>>
>> Needless to say, this would be a breaking change and only possible in
>> Airflow 2.0.
>>
>> Cheers,
>> Bas
>>
>
>
> --
>
> Jarek Potiuk
> Polidea <https://www.polidea.com/> | Principal Software Engineer
>
> M: +48 660 796 129 <+48660796129>
> [image: Polidea] <https://www.polidea.com/>
Re: AIP-22: Group ORM models by their logical usage instead of type
Posted by Jarek Potiuk <Ja...@polidea.com>.
Just few comments to spark a discussion about backwards compatibility for
this proposal:
- Airflow uses Alembic to generate automated migration scripts. Not sure
where it takes all the models from to generate the scripts (__init__ in
airflow.models maybe, or simply by scanning all classes). I do not expect
any serious problems as alembic uses column/table names in its migration
scripts but it would be worth checking if there are any negative
consequences
- I think it might make sense to keep airflow.models.__init__ in place
anyway to keep backwards compatibility maybe ?
- I think there is one particular case which is super-sensitive for
backwards compatibility models.DAG - which is used pretty much in all DAGs
written by anyone (`from airflow.models import DAG` or `with
models.DAG(...` ). I think it's one thing to break some backwards
compatibility in edge cases and it's quite a different thing if pretty much
all custom-written DAGs have to be corrected. I think this is yet another
argument to keep at least the old __init__ and import the classes from
their new locations still keeping the old 'models' package - even if we
decide to move them. And then it might turn out the change will be fully
backwards-compatible.
J.
On Sat, Jun 22, 2019 at 8:50 PM Bas Harenslak <ba...@godatadriven.com>
wrote:
> Hi all,
>
> I’ve created AIP-22 and propose to move every ORM class to a module where
> one would logically expect them to be, instead of grouping all ORM models
> together, e.g. BaseOperator in airflow.operators (this is actually not an
> ORM class).
>
> To me the benefit would be a more logically structured code base, with
> classes stored where you’d initially expect them to be, instead of having
> to know upfront if they’re ORM classes or not.
>
> I’ve explained the plan in more detail in the AIP:
> https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-22%3A+Group+ORM+models+by+their+logical+usage+instead+of+type
> <
> https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-22:+Group+ORM+models+by+their+logical+usage+instead+of+type>
> and wonder if others think the same about it.
>
> Needless to say, this would be a breaking change and only possible in
> Airflow 2.0.
>
> Cheers,
> Bas
>
--
Jarek Potiuk
Polidea <https://www.polidea.com/> | Principal Software Engineer
M: +48 660 796 129 <+48660796129>
[image: Polidea] <https://www.polidea.com/>