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 <[email protected]> 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 <[email protected]> > 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 <[email protected]> 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 <[email protected]> >> 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 <[email protected]> >> 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 <[email protected]> >> 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 < >> [email protected]> >> > >>> 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 < >> [email protected]> >> > >>>> 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 < >> > >>>> [email protected]> >> > >>>>> wrote: >> > >>>>> >> > >>>>>> Inline comment >> > >>>>>> >> > >>>>>> On Fri, Apr 19, 2019 at 12:23 AM Arthur Wiedmer < >> > >>>>> [email protected]> >> > >>>>>> 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: [email protected] >> > >>>>>> [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: [email protected] >> > >>>>> >> > >>>> >> > >>>> >> > >>>> -- >> > >>>> >> > >>>> Kamil Breguła >> > >>>> Polidea <https://www.polidea.com/> | Software Engineer >> > >>>> >> > >>>> M: +48 505 458 451 <+48505458451> >> > >>>> E: [email protected] >> > >>>> [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: [email protected] >> > >> >> > >> >> > > >> > > -- >> > > >> > > Kamil Breguła >> > > Polidea <https://www.polidea.com/> | Software Engineer >> > > >> > > M: +48 505 458 451 <+48505458451> >> > > E: [email protected] >> > > [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/>
