Valid points Sergio. I changed my mind and agree that getting rid of contrib is a better idea.
On Thu, Jul 25, 2019 at 7:39 PM Sergio Kef <sergio...@gmail.com> wrote: > My 2 cents regarding contrib. > > I don't see many benefits of keeping a contrib module. As a user it is > super confusing that for example we have 2 slack operators. one in main > operators, another in contrib. > > The idea that we add experimental features is fine, but in this case it > lacks of good implementation (example airflow.operators.S3_hook now uses > airflow.contrib.hooks.aws_hook). Also, I think feature selection should not > be based on "maturity" but on project strategy as to what is "core" airflow > and what it's not. > > This opens the space to discuss about whether it makes sense for all those > operators and hooks to be part of the main airflow project - or would it be > better to create an "airflow-contrib" project where experimental plugins > can be added (sets of hooks, operators, views, macros, etc - eg AWS plugin > can get the whole package of dependencies, hooks and operators). This > brings some benefits - eg easier deployment, faster testing and also some > challenges, like what do we do with operators that use totally different > hooks. > > So if something is not well tested, let's test it, if it's not up to our > standards let's create tickets and improve it. If it's not useful enough, > lets remove it (and potentially create something like airflow-contrib with > experimental plugins or similar). Having it in a different folder doesn't > improve the quality somehow, neither makes development more agile, it just > sweeps the problems under the carpet and creates an extra problem of > inconsistency. > > S. > > On Wed, 17 Jul 2019 at 08:13, Jarek Potiuk <jarek.pot...@polidea.com> > wrote: > > > Since we are just about to release 1.10.4 and my team is gearing up to > some > > "summer" cleanup of GCP operators - I would like to revive this thread > and > > reach some binding conclusions on this finally > > > > Kamil prepared this document: > > > > > https://docs.google.com/document/d/1F8zve5S78DXcjpPttW89HnqT0M0iKjT6fo9jX57Ef6A/edit# > > describing > > the ideas around the new structure/naming of operators/hooks (with > > backwards compatibility mechanisms). > > > > Hopefully there are no big contentions and even if there are some > different > > opinions, they are not too strong to block us from reaching consensus. > > > > From what I saw in this thread this is the preferred solution: > > > > - *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)* > > > > Regarding deprecation mechanism - there are some different opinions on > zope > > vs. custom deprecation but I think we can leave decision there to the > first > > PR time when we see it actually working and how much code it adds. > > > > If there are no big "noooooo " within a day, I will convert it into > > (smaller and official) AIP proposal, run a vote and make JIRA issues out > of > > that. > > > > J. > > > > On Mon, Jun 17, 2019 at 1:37 PM Jarek Potiuk <jarek.pot...@polidea.com> > > wrote: > > > > > Yeah. I think it's worth even if it's a bit of boilerplate code. I > think > > > we might be a bit more friendly and welcoming to new/accidental users > who > > > just want to make a small PR and are not regular users. Those people > will > > > often use IDEs (and Pycharm is by far the most popular in python > world). > > > Similarly to AIP-7 I think if those people have lower barrier entry and > > > better guidance, we will only all benefit from that :). > > > > > > J. > > > > > > On Tue, Jun 11, 2019 at 8:53 PM Kamil Breguła < > kamil.breg...@polidea.com > > > > > > wrote: > > > > > >> If I added a warning in the method that it calls immediately, IDE > > >> still could not detect everything correctly. The user is not able to > > >> see the error message in the IDE. The warning appears in the wrong > > >> place. > > >> https://i.imgur.com/CSqP1wU.png > > >> > > >> Adding a new import allows you to avoid some IDE warnings, but the > > >> problem remains that this module is not detected as deprecated > > >> https://i.imgur.com/CSqP1wU.png > > >> . > > >> > > >> On Thu, Jun 6, 2019 at 11:32 AM Ash Berlin-Taylor <a...@apache.org> > > wrote: > > >> > > > >> > I hate IDEs :( > > >> > > > >> > What does the IDE show if you package up the deprecation warning in > > >> some way in your solution (i.e. the warning.warn isn't directly in the > > >> module source)? Alternatively adding `from solution2.new import *` > into > > >> solution2.deprecated may help the IDE? > > >> > > > >> > -a > > >> > > > >> > > On 5 Jun 2019, at 17:41, Kamil Breguła <kamil.breg...@polidea.com > > > > >> wrote: > > >> > > > > >> > > I have done a deep investigation of this issue. Please, look at > the > > >> > > beginning of chapter "Case #7". > > >> > > > > >> > > I prepared a repository with simple demo and also shared a > > screenshot > > >> from > > >> > > my IDE. > > >> > > Photo: https://imgur.com/a/mRaWpQm > > >> > > Repo: https://github.com/mik-laj/airflow-deprecation-sample > > >> > > Today I looked again at this project. I recently updated the IDE > to > > >> the > > >> > > latest version - IntelliJ IDEA 2019.1.2 (Ultimate Edition). The > > issue > > >> still > > >> > > exists. > > >> > > I made additional photos today: https://imgur.com/a/4fcmkVX > > >> > > > > >> > > IDE uses statistical analysis of the code and gets lost in > external > > >> > > libraries. > > >> > > > > >> > > If you have questions, I will be happy to give you further > > >> explanations. > > >> > > > > >> > > On Wed, Jun 5, 2019 at 4:30 PM Ash Berlin-Taylor <a...@apache.org> > > >> wrote: > > >> > > > > >> > >>> Case 7: Zope deprecation or not > > >> > >>> *B*: I prefer to use Kamil's in-house hack than zope, but I feel > > we > > >> could > > >> > >>> improve it slightly to remove a bit of boilerplate - it looks > like > > >> the > > >> > >>> warning.warn() message should be very similar in all deprecation > > >> cases > > >> > >> and > > >> > >>> possibly we can define it in one place (providing that > > >> DepracationWarning > > >> > >>> will continue to work well with IDEs) > > >> > >> > > >> > >> At this point wouldn't or home-rolled solution be very similar to > > >> > >> zope.deprecation (which is already a dep of Airflow)? > > >> > >> > > >> > >> Also are we sure that zope.deprecation isn't supported by an IDE? > > It > > >> still > > >> > >> issues a warning.warn. (The example docs in zope.deprecation show > > >> messing > > >> > >> about with sys.modules but that isn't needed.) > > >> > >> > > >> > >> https://gist.github.com/ashb/327a79d7c6c1f5a2344cd1812a8ee92d > > shows > > >> a > > >> > >> formatted example: > > >> > >> > > >> > >> ``` > > >> > >> #Place this as airflow/foo.py > > >> > >> import zope.deprecation > > >> > >> zope.deprecation.moved('airflow.settings', 'version 2') > > >> > >> ``` > > >> > >> > > >> > >> And then when you import that module you get: > > >> > >> > > >> > >> ``` > > >> > >> /Users/ash/.virtualenvs/airflow/bin/ipython:1: > DeprecationWarning: > > >> > >> airflow.foo has moved to airflow.settings. Import of airflow.foo > > will > > >> > >> become unsupported in version 2 > > >> > >> #!/Users/ash/.virtualenvs/airflow/bin/python3.7 > > >> > >> > > >> > >> ``` > > >> > >> > > >> > >> That should work fine with IDE? > > >> > >> > > >> > >> > > >> > >>> On 5 Jun 2019, at 14:56, Jarek Potiuk <jarek.pot...@polidea.com > > > > >> wrote: > > >> > >>> > > >> > >>> I think it's very important to discuss this and choose the right > > >> approach > > >> > >>> and follow up quickly. This is very important to introduce some > > >> sanity in > > >> > >>> the structure of 2.0.0. > > >> > >>> I propose that anyone with a strong opinion voices his or her > > >> concerns > > >> > >> here > > >> > >>> and then maybe a week from now we can get to a consensus and > vote > > >> on the > > >> > >>> direction we take? > > >> > >>> > > >> > >>> My preferences (but I can be convinced to change some of my > > >> choices) are: > > >> > >>> > > >> > >>> Case 1: (contrib vs not) > > >> > >>> *B* (move all that are "well" tested): I think having > "incubator" > > >> kind of > > >> > >>> approach for not super tested/documented but still usable code > is > > >> useful. > > >> > >>> Though we would have to establish some objective criteria and > > >> describe a > > >> > >>> "growing from incubation" process. > > >> > >>> > > >> > >>> Case 2 airflow.operators.foo_operator.FooOperator could become > > >> > >>> *A:* airflow.operators.foo.FooOperator > > >> > >>> > > >> > >>> Case 3: > > >> airflow.contrib.operators.gcp_bigtable_operator.BigTableOperator > > >> > >>> could become > > >> > >>> *C*: airflow.integration.gcp.operators.bigtable.BigTableOperator > > >> > >>> > > >> > >>> Case 4: (separate packages/namesapces) > > >> > >>> *B:* no namespaces - i think we are very far from modularisation > > to > > >> > >> happen > > >> > >>> (and how) and it might get never implemented so there are no > real > > >> > >> benefits > > >> > >>> I see now. > > >> > >>> > > >> > >>> Case 5: "Operator" suffix on class names > > >> > >>> *B*: I prefer to keep B as this will be much easier when you > > search > > >> for > > >> > >>> class using your IDE. > > >> > >>> > > >> > >>> Case 6: "Isolated" renames > > >> > >>> *Rename*: We should align names of those operators. > > >> > >>> > > >> > >>> Case 7: Zope deprecation or not > > >> > >>> *B*: I prefer to use Kamil's in-house hack than zope, but I feel > > we > > >> could > > >> > >>> improve it slightly to remove a bit of boilerplate - it looks > like > > >> the > > >> > >>> warning.warn() message should be very similar in all deprecation > > >> cases > > >> > >> and > > >> > >>> possibly we can define it in one place (providing that > > >> DepracationWarning > > >> > >>> will continue to work well with IDEs) > > >> > >>> > > >> > >>> J, > > >> > >>> > > >> > >>> On Tue, Jun 4, 2019 at 2:11 PM Kamil Breguła < > > >> kamil.breg...@polidea.com> > > >> > >>> wrote: > > >> > >>> > > >> > >>>> I created AIP-21 > > >> > >>>> > > >> > >>>> > > >> > >> > > >> > > > https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-21%3A+Changes+in+import+paths > > >> > >>>> This is the same document as above, I just deleted my opinions > > >> (summary > > >> > >>>> sections) and work plan. > > >> > >>>> > > >> > >>>> > > >> > >>>> On Fri, Apr 19, 2019 at 2:18 PM Jarek Potiuk < > > >> jarek.pot...@polidea.com> > > >> > >>>> wrote: > > >> > >>>> > > >> > >>>>> Thanks Kamil! Excellent analysis! > > >> > >>>>> > > >> > >>>>> My preferences: > > >> > >>>>> > > >> > >>>>> Case 1: (contrib vs not) > > >> > >>>>> B (move all that are "well" tested): I think having > "incubator" > > >> kind of > > >> > >>>>> approach for not super tested/documented but still usable code > > is > > >> > >> useful. > > >> > >>>>> Though we would have to establish some objective criteria and > > >> describe > > >> > >> a > > >> > >>>>> "growing from incubation" process. > > >> > >>>>> > > >> > >>>>> Case 2 airflow.operators.foo_operator.FooOperator could become > > >> > >>>>> A: airflow.operators.foo.FooOperator > > >> > >>>>> > > >> > >>>>> Case 3: > > >> > >> airflow.contrib.operators.gcp_bigtable_operator.BigTableOperator > > >> > >>>>> could become > > >> > >>>>> C: airflow.integration.gcp.operators.bigtable.BigTableOperator > > >> > >>>>> > > >> > >>>>> Case 4: (separate packages/namesapces) > > >> > >>>>> B: no namespaces - i think we are very far from modularisation > > to > > >> > >> happen > > >> > >>>>> (and how) and it might get never implemented so there are no > > real > > >> > >>>> benefits > > >> > >>>>> I see now. > > >> > >>>>> > > >> > >>>>> Case 5: "Operator" suffix on class names > > >> > >>>>> B: I prefer to keep B as this will be much easier when you > > search > > >> for > > >> > >>>> class > > >> > >>>>> using your IDE. > > >> > >>>>> > > >> > >>>>> Case 7: Zope deprecation or not > > >> > >>>>> B: I prefer to use Kamil's in-house hack than zope, but I feel > > we > > >> could > > >> > >>>>> improve it slightly to remove a bit of boilerplate - it looks > > >> like the > > >> > >>>>> warning.warn() message should be very similar in all > deprecation > > >> cases > > >> > >>>> and > > >> > >>>>> possibly we can define it in one place (providing that > > >> > >> DepracationWarning > > >> > >>>>> will continue to work well with IDEs) > > >> > >>>>> > > >> > >>>>> > > >> > >>>>> > > >> > >>>>> > > >> > >>>>> On Fri, Apr 19, 2019 at 12:36 AM Kamil Breguła < > > >> > >>>> kamil.breg...@polidea.com> > > >> > >>>>> wrote: > > >> > >>>>> > > >> > >>>>>> Inline comment > > >> > >>>>>> > > >> > >>>>>> On Fri, Apr 19, 2019 at 12:23 AM Arthur Wiedmer < > > >> > >>>>> arthur.wied...@gmail.com> > > >> > >>>>>> wrote: > > >> > >>>>>> > > >> > >>>>>>> > > >> > >>>>>>>>>>> Case #5 Are we talking about the class name or the file > > >> name? > > >> > >>>>> The > > >> > >>>>>>>> class > > >> > >>>>>>>>>>> name is fine to me, but we could remove _operator from > the > > >> > >>>> file > > >> > >>>>>>> name. > > >> > >>>>>>>>>>> > > >> > >>>>>>>>>> > > >> > >>>>>>>>>> Case #2 describes the change of file names > > >> > >>>>>>>>>> Case #5 describes the change of class names > > >> > >>>>>>>>>> > > >> > >>>>>>>>>> I added examples to two cases to better describe the > > changes. > > >> > >>>>>>>>>> > > >> > >>>>>>>>> > > >> > >>>>>>>>> Thank you. I guess I see it for file names, but I am > > wondering > > >> > >>>>> about > > >> > >>>>>>> the > > >> > >>>>>>>>> operators and name collisions. > > >> > >>>>>>>>> Say I need both the HiveOperator and I inline a custom > > >> operator > > >> > >>>> for > > >> > >>>>>>>> which I > > >> > >>>>>>>>> need a hive hook. > > >> > >>>>>>>>> # Would you recommmend the following? Or does case #5 only > > >> apply > > >> > >>>> to > > >> > >>>>>>>>> Operators? > > >> > >>>>>>>>> from airflow.operators.hive import Hive as HiveOperator > > >> > >>>>>>>>> from airfow.hooks.hive import Hive as HiveHook > > >> > >>>>>>>>> > > >> > >>>>>>>>> I was just thinking it might be nice to be able to import > > >> what I > > >> > >>>>> need > > >> > >>>>>>>>> without renaming and not worry too much about names > > shadowing > > >> > >>>>> others. > > >> > >>>>>>>>> Especially if I am new-ish to Apache Airflow. > > >> > >>>>>>>>> > > >> > >>>>>>>>> > > >> > >>>>>>>> I think that the operator should describe the > behavior(verb), > > >> hook > > >> > >>>>>> should > > >> > >>>>>>>> describe a service(noun). These are other parts of speech. > > >> > >>>>>>>> In my opinion, HiveOperator should be named > HiveExecuteQuery. > > >> Hook > > >> > >>>>> will > > >> > >>>>>>> be > > >> > >>>>>>>> called Hive. Then there will be no name conflicts. > > >> > >>>>>>>> > > >> > >>>>>>> > > >> > >>>>>>> But we will still provide the backwards compatibility for a > > >> while > > >> > >>>> with > > >> > >>>>>>> aliases to the old names, correct? > > >> > >>>>>>> > > >> > >>>>>> > > >> > >>>>>> Yes. It is possible to provide backward compatibility also in > > >> this > > >> > >>>> case. > > >> > >>>>>> > > >> > >>>>>> > > >> > >>>>>> -- > > >> > >>>>>> > > >> > >>>>>> Kamil Breguła > > >> > >>>>>> Polidea <https://www.polidea.com/> | Software Engineer > > >> > >>>>>> > > >> > >>>>>> M: +48 505 458 451 <+48505458451> > > >> > >>>>>> E: kamil.breg...@polidea.com > > >> > >>>>>> [image: Polidea] <https://www.polidea.com/> > > >> > >>>>>> > > >> > >>>>>> We create human & business stories through technology. > > >> > >>>>>> Check out our projects! <https://www.polidea.com/our-work> > > >> > >>>>>> [image: Github] <https://github.com/Polidea> [image: > Facebook] > > >> > >>>>>> <https://www.facebook.com/Polidea.Software> [image: Twitter] > > >> > >>>>>> <https://twitter.com/polidea> [image: Linkedin] > > >> > >>>>>> <https://www.linkedin.com/company/polidea> [image: > Instagram] > > >> > >>>>>> <https://instagram.com/polidea> [image: Behance] > > >> > >>>>>> <https://www.behance.net/polidea> > > >> > >>>>>> > > >> > >>>>> > > >> > >>>>> > > >> > >>>>> -- > > >> > >>>>> > > >> > >>>>> Jarek Potiuk > > >> > >>>>> Polidea <https://www.polidea.com/> | Principal Software > > Engineer > > >> > >>>>> > > >> > >>>>> M: +48 660 796 129 <+48660796129> > > >> > >>>>> E: jarek.pot...@polidea.com > > >> > >>>>> > > >> > >>>> > > >> > >>>> > > >> > >>>> -- > > >> > >>>> > > >> > >>>> Kamil Breguła > > >> > >>>> Polidea <https://www.polidea.com/> | Software Engineer > > >> > >>>> > > >> > >>>> M: +48 505 458 451 <+48505458451> > > >> > >>>> E: kamil.breg...@polidea.com > > >> > >>>> [image: Polidea] <https://www.polidea.com/> > > >> > >>>> > > >> > >>>> We create human & business stories through technology. > > >> > >>>> Check out our projects! <https://www.polidea.com/our-work> > > >> > >>>> [image: Github] <https://github.com/Polidea> [image: Facebook] > > >> > >>>> <https://www.facebook.com/Polidea.Software> [image: Twitter] > > >> > >>>> <https://twitter.com/polidea> [image: Linkedin] > > >> > >>>> <https://www.linkedin.com/company/polidea> [image: Instagram] > > >> > >>>> <https://instagram.com/polidea> [image: Behance] > > >> > >>>> <https://www.behance.net/polidea> > > >> > >>>> > > >> > >>> > > >> > >>> > > >> > >>> -- > > >> > >>> > > >> > >>> Jarek Potiuk > > >> > >>> Polidea <https://www.polidea.com/> | Principal Software > Engineer > > >> > >>> > > >> > >>> M: +48 660 796 129 <+48660796129> > > >> > >>> E: jarek.pot...@polidea.com > > >> > >> > > >> > >> > > >> > > > > >> > > -- > > >> > > > > >> > > Kamil Breguła > > >> > > Polidea <https://www.polidea.com/> | Software Engineer > > >> > > > > >> > > M: +48 505 458 451 <+48505458451> > > >> > > E: kamil.breg...@polidea.com > > >> > > [image: Polidea] <https://www.polidea.com/> > > >> > > > > >> > > We create human & business stories through technology. > > >> > > Check out our projects! <https://www.polidea.com/our-work> > > >> > > [image: Github] <https://github.com/Polidea> [image: Facebook] > > >> > > <https://www.facebook.com/Polidea.Software> [image: Twitter] > > >> > > <https://twitter.com/polidea> [image: Linkedin] > > >> > > <https://www.linkedin.com/company/polidea> [image: Instagram] > > >> > > <https://instagram.com/polidea> [image: Behance] > > >> > > <https://www.behance.net/polidea> > > >> > > > >> > > > > > > > > > -- > > > > > > 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/> > > > -- Jarek Potiuk Polidea <https://www.polidea.com/> | Principal Software Engineer M: +48 660 796 129 <+48660796129> [image: Polidea] <https://www.polidea.com/>
