airflow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Philippe Gagnon <philgagn...@gmail.com>
Subject Re: [2.0 spring cleaning] Changes in import paths
Date Thu, 06 Jun 2019 02:25:57 GMT
Hello community,

Here is my take on this:

Case #1:

Solution B, but I would like for us to consider putting incubating modules
in a separate repository instead of vendoring them with Airflow itself.

There are pros and cons to this approach. On one hand, I think that we need
to take responsibility for all code in the main repo, and this would help
us focus our efforts on high-quality, well-tested packages. However, this
would have the effect that incubating packages would see less usage, and
therefore might develop more slowly.

Case #2:

Solution A.

Case #3:

Solution C, however I do not see it as incompatible with Case #1 Solution B.

Case #4:

Solution B, but I think that we could consider namespacing incubating
packages if solution B is retained for Case #1.

Case #5:

Solution B. When in doubt, I would rather err on the side of explicitness.

Case #6:

Agreed that we need to be consistent in naming. Help from CSPs would be
very good.

Case #7:

No opinion.

Regards,

Philippe

On Wed, Jun 5, 2019 at 12:42 PM Kamil Breguła <kamil.bregula@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 <ash@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.Potiuk@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.bregula@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.Potiuk@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.bregula@polidea.com>
> > >>> wrote:
> > >>>
> > >>>> Inline comment
> > >>>>
> > >>>> On Fri, Apr 19, 2019 at 12:23 AM Arthur Wiedmer <
> > >>> arthur.wiedmer@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.bregula@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.potiuk@polidea.com
> > >>>
> > >>
> > >>
> > >> --
> > >>
> > >> Kamil Breguła
> > >> Polidea <https://www.polidea.com/> | Software Engineer
> > >>
> > >> M: +48 505 458 451 <+48505458451>
> > >> E: kamil.bregula@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.potiuk@polidea.com
> >
> >
>
> --
>
> Kamil Breguła
> Polidea <https://www.polidea.com/> | Software Engineer
>
> M: +48 505 458 451 <+48505458451>
> E: kamil.bregula@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>
>

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