airflow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Standish <dpstand...@gmail.com>
Subject Re: [VOTE] Changes in import paths
Date Fri, 26 Jul 2019 17:38:29 GMT
I have tried to add some clarification to Jarek's summary, but I am a
little fuzzy on exact proposal for case 3 so hopefully Jarek can further
clarify.

According to my reading, in this motion cases 4,5,6 are all either proposal
*rejections* or otherwise have no effect, so attention can be focused on
cases 1,2,3 and 7.

*Motion is to adopt the following:*

Case 1 - Solution A - Get rid of contrib
All objects will be moved out of contrib folder and into respective
non-contrib folder.

Case 2 - Solution A - Remove object type suffix from modules
Example:
Airflow.operators.foo_operator.FooOperator becomes
airflow.operators.foo.FooOperator
Example:
Airflow.hooks.foo_hook.FooOperator becomes airflow.hooks.foo.FooHook

Case 3 - conventions for cloud provider etc prefixes
*I am not sure exactly what the proposal is.*
Example case is airflow/contrib/operators/gcp_bigtable_operator.py
Since motion adopts case 1 solution A, we can assume it is now named like
so: airflow/operators/gcp_bigtable_operator.py
So what is proposed for this case?

Case 4 - Solution B - *no change*
This proposal was about namespaces but the motion is to reject the proposal.

Case 5 - Solution B - *no change*
The proposal was to remove class type suffix e.g. remove the Operator in
FooOperator, but the motion is to reject this proposal -- i.e. motion is no
change on this case, i.e. we resolve to keep "Operator" (and "Sensor")
suffixes on class names

Case 6 - *No change*
This case concerns object naming inconsistencies, but motion does not enact
any specific proposal.

Case 7 - Solution A - use warnings.warn template for deprecation warnings
(instead of zope)
Example:

# in old_package/old_mod.py

import warnings
from new_package.new_mod import *
warnings.warn("old_package.old_mod has moved to new_package.new_mod. Import
of "
"old_package.old_mod will become unsupported in version 2",
DeprecationWarning, 2)

Reference implementation here:
https://github.com/mik-laj/airflow-deprecation-sample/tree/master/solution1


FWIW +1 (non-binding) on 1,2,7 -- unsure on 3.

I am very happy that this motion now gets rid of contrib.  Thank you Sergio
for turning the tide with your effective argumentation ;)





On Fri, Jul 26, 2019 at 3:16 AM Ash Berlin-Taylor <ash@apache.org> wrote:

> Could someone summarise what the proposed name changes are, here, in this
> thread? Pointing at a google doc and only giving partial examples is 1) a
> bit difficult to quickly work out what we are voting on, and 2) using a
> google doc isn't great for a longer term record.
>
> Thanks,
> Ash
>
> > On 26 Jul 2019, at 09:14, Jarek Potiuk <Jarek.Potiuk@polidea.com> wrote:
> >
> > I would like to recast the vote. Let's start from 0 :) . I would like to
> > keep the July 30th 6pm CEST date, and at least three +1 (binding) votes
> are
> > needed to pass. I marked in red the modifications to the previous
> proposal.
> >
> > Here is the proposal (details here
> > <
> https://docs.google.com/document/d/1F8zve5S78DXcjpPttW89HnqT0M0iKjT6fo9jX57Ef6A/edit#heading=h.j4jc3i70qszo
> >
> > )
> >
> >   - *Case 1: A: Get rid of contrib,*
> >   - *Case 2: A: Airflow.operators.foo_operator.FooOperator could
> >   become airflow.operators.foo.FooOperator*
> >   - *Case 3: C:
> >   airflow.contrib.operators.gcp_bigtable_operator.BigTableOperator could
> >   become airflow.gcp.operators.bigtable.BigTableOperator*
> >   - *Case 4: B: no namespace introduction*
> >   - *Case 5: B: Keep "Operator" (and "Sensor") suffixes on class names*
> >   - *Case 6: We will treat isolated cases on case-by-case (and my team
> can
> >   do the job of GCP-related operators)*
> >   - *Case 7: Native python solution (with better IDE integration)*
> >
> > This is my binding (+1) vote.
> >
> >
> >
> > On Fri, Jul 26, 2019 at 10:08 AM Jarek Potiuk <Jarek.Potiuk@polidea.com>
> > wrote:
> >
> >> Hey Felix,
> >>
> >> For 7* -> I am in favour of trying the native one as well (I use IDE
> quite
> >> often ;) )
> >>
> >> J.
> >>
> >> On Wed, Jul 24, 2019 at 9:34 AM Driesprong, Fokko <fokko@driesprong.frl
> >
> >> wrote:
> >>
> >>> Thanks Kamil for putting the document together. I wasn't able to
> respond
> >>> earlier since I was giving Airflow workshops throughout Europe :-)
> >>>
> >>> *Case 1: *Solution A → Remove everything from contrib into a single
> >>> package.
> >>>
> >>> To make it easier to the end-user, my preference would be to get rid of
> >>> the
> >>> contrib package altogether. What's in contrib or not is a tricky
> question,
> >>> I think this is already reflected in the document since "maturity" is
> in
> >>> quotes. What would be the moment to transfer the operator to the core
> or
> >>> vice versa? I'm afraid that renaming contrib to incubating will confuse
> >>> the
> >>> user even more. For people who aren't as known with Airflow it isn't
> that
> >>> transparent which operator lives where, especially if you don't use an
> IDE
> >>> such as Ash ;) Besides that I don't think it is worth changing the
> public
> >>> API just for a different name.
> >>>
> >>
> >> Ok. I am convinced. I will re-cast the vote with this. It will make
> easier
> >> as we will not have to define other rules as those that we already have.
> >>
> >>
> >>> *Case 2:* Slight preference to Solution B (let's say a +0)
> >>>
> >>> I like to search on Github with t, and then search for BashOperator.
> After
> >>> the change, you should search for Operator/Bash.
> >>>
> >>> Jarek, I'm confused with your vote on this, from your mail you state: -
> >>> *Case 2: B: Airflow.operators.foo_operator.FooOperator could become
> >>> airflow.operators.foo.FooOperator*. But the B solution is keeping it
> the
> >>> same, so that would mean - *Case 2: B:
> >>> Airflow.operators.foo_operator.FooOperator *would stay
> >>> airflow.operators.foo_operator*.FooOperator*
> >>>
> >>> My mistake. It was of course A. I think there is a little confusion
> here.
> >> This case is not for changing BashOperator into Bash but for changing
> the
> >> *module* name not the *class* name. So bash_operator.BashOperator ->
> >> bash.BashOperator :) . you will still search for BashOperator in this
> case.
> >>
> >>
> >>> *Case 3:* I'm leaning towards B
> >>>
> >>> Mostly because it isn't as trivial to decide when it gets its own
> package
> >>> or not. Besides the cloud providers, there are companies like
> Databricks
> >>> and Qubole which might also end up with their own package because their
> >>> integration is getting richer over time. Moving this is a bit painful
> >>> because we need to deprecate the old path and move everything which
> >>> introduces noise into version control etc.
> >>
> >>
> >> I'd say, we should go for C. As I proposed. It's not as difficult as it
> >> seems to group operators in packages and it has numerous advantages. The
> >> nice thing about deprecations - we can do them in the __init__.py of the
> >> packages which causes the noise fairly small (Git will actually realise
> >> that the file has been moved and you can follow that. Maybe not as super
> >> easy to merge changes later to 1.10.4  but it's not a rocket science
> either.
> >>
> >>
> >>> *Case 7:* Python native solution
> >>>
> >>
> >> Yes.
> >>
> >>
> >>>
> >>> ---
> >>>
> >>> Apart from all, great work!
> >>>
> >>> Cheers, Fokko
> >>>
> >>>
> >>> Op di 23 jul. 2019 om 21:27 schreef Felix Uellendall
> >>> <feluelle@pm.me.invalid
> >>>> :
> >>>
> >>>> I have no strong "No" against any proposed change of these cases. So
I
> >>> go
> >>>> with +1 (non-binding).
> >>>>
> >>>> P.S. Thanks Jarek for bringing this up again and your intense work
> >>> towards
> >>>> airflow currently :) and thanks to Kamil for even creating this
> >>> document. I
> >>>> like how the code is getting more and more consistent and clean :)
> >>>>
> >>>> Kind regards,
> >>>> Felix
> >>>>
> >>>> Sent from ProtonMail mobile
> >>>>
> >>>> -------- Original Message --------
> >>>> On Jul 23, 2019, 18:34, Jarek Potiuk wrote:
> >>>>
> >>>>> Hello everyone,
> >>>>>
> >>>>> This email is calling a vote on the changes in import paths. It's
> been
> >>>>> discussed in
> >>>>>
> >>>>
> >>>
> https://lists.apache.org/thread.html/4e648d9421c792d4537f5ac66f1a16dce468f816fc5221a9f9db9433@%3Cdev.airflow.apache.org%3E
> >>>>>
> >>>>> The vote will last for at least 1 week (July 30th 6pm CEST), and
at
> >>> least
> >>>>> three +1 (binding) votes have been cast.
> >>>>>
> >>>>> The proposal to vote is based on the document from Kamil
> >>>>> <
> >>>>
> >>>
> https://docs.google.com/document/d/1F8zve5S78DXcjpPttW89HnqT0M0iKjT6fo9jX57Ef6A/edit#
> >>>>>
> >>>>>
> >>>>> The proposed solution is:
> >>>>>
> >>>>> - *Case 1: B: Contrib vs not: we move all that are "well" tested
and
> >>>>> rename contrib to "incubating" or similar.*
> >>>>> - *Case 2: B: Airflow.operators.foo_operator.FooOperator could
> >>>>> become airflow.operators.foo.FooOperator*
> >>>>> - *Case 3: C:
> >>>>> airflow.contrib.operators.gcp_bigtable_operator.BigTableOperator
> could
> >>>>> become airflow.gcp.operators.bigtable.BigTableOperator*
> >>>>> - *Case 4: B: no namespace introduction*
> >>>>> - *Case 5: B: Keep "Operator" (and "Sensor") suffixes on class names*
> >>>>> - *Case 6: We will treat isolated cases on case-by-case (and my
team
> >>> can
> >>>>> do the job of GCP-related operators)*
> >>>>>
> >>>>> This is my (binding) +1 vote.
> >>>>>
> >>>>> Best regards,
> >>>>>
> >>>>> J.
> >>>>>
> >>>>> --
> >>>>>
> >>>>> Jarek Potiuk
> >>>>> Polidea <https://www.polidea.com/> | Principal Software Engineer
> >>>>>
> >>>>> M: [+48 660 796 129](tel:+48660796129)
> >>> <[+48660796129](tel:+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/>
> >>
> >>
> >
> > --
> >
> > Jarek Potiuk
> > Polidea <https://www.polidea.com/> | Principal Software Engineer
> >
> > M: +48 660 796 129 <+48660796129>
> > [image: Polidea] <https://www.polidea.com/>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message