Thanks Kamil! You rock
https://github.com/apache/airflow/pull/7517
On Feb 23 2020, at 10:36 pm, Ash Berlin-Taylor <a...@apache.org> wrote:
> Yeah both Pycharm and VSCode are perfectly happy with that (and jump to
> impl/defintion works right which is better than the type stub version too)
>
> PR incoming. I don't think we need to worry about the method Celery is going
> to as (I think) Py3.6 supports PEP-562 natively.
> -ash
> On Feb 23 2020, at 10:30 pm, Ash Berlin-Taylor <a...@apache.org> wrote:
> > Ahha, I was wondering if there was something like typing.TYPE_CHECKING but
> > for static analyzers/IDEs. I like that approach. Testing it out now.
> >
> > On Feb 23 2020, at 10:28 pm, Kamil Breguła <kamil.breg...@polidea.com>
> > wrote:
> > > Hello,
> > >
> > > I admit that I do not follow the subject closely, but I think that
> > > here is the answer.
> > > https://github.com/celery/celery/blob/master/celery/__init__.py#L63-L78
> > >
> > > We can also create a module with lazy-loaded attributes:
> > > https://github.com/celery/celery/blob/master/celery/__init__.py#L158-L184
> > >
> > > Best regards,
> > > Kamil
> > >
> > > On Sun, Feb 23, 2020 at 11:20 PM Ash Berlin-Taylor <a...@apache.org>
> > > wrote:
> > > >
> > > > I've managed to get type stubs working in VSCode (and intellij), so
> > > > that may be an option.
> > > > TBH I think I'm leaning somewhat towards 5 -- after all that would then
> > > > go some way to clarify the distinction between DAG and DagModel (i.e.
> > > > DAG doesn't belong in models, it's not a DB model)
> > > > On Feb 23 2020, at 10:15 pm, Kaxil Naik <kaxiln...@gmail.com> wrote:
> > > > > 1. Just have a deprecation warning (which it's not even clear if
> > > > > PEP-562
> > > > > style will show a nice (!) warning in IDEs or just at run time
> > > > > 2. I learn to live with from airflow.models.dag import DAG
> > > > > 3. After we change airflow.models to not import all the sub-modules
> > > > > automatically, go back to importing "airflow.models.dag.DAG" into
> > > > > "airflow".
> > > > > 4. We create a new (abstract) base class that we _can_ import in to
> > > > > the
> > > > > airflow module that airflow.models.dag.DAG extends. This could be
> > > > > automated
> > > > > by a pre-commit so it is always up to date with the real DAG class
> > > > > 5. We create a new module of things that are part of the public/dag
> > > > > author API, such as airflow.dag (currently just has a "BaseDAG"
> > > > > sub-module
> > > > > but that is barely used, and only by the scheduler so could be
> > > > > removed)
> > > > >
> > > > >
> > > > > Happy with (3) and/or (5)
> > > > > (1) does not work with Pycharm - it doesn't show deprecation warning.
> > > > > ᐧ
> > > > >
> > > > > On Sun, Feb 23, 2020 at 10:10 PM Ash Berlin-Taylor <a...@apache.org>
> > > > > wrote:
> > > > > > Gah not only that, vscode also ignores __all__ settings which is
> > > > > > really
> > > > > > bad form.
> > > > > >
> > > > > > Pycharm at least does respect __all__ for what to show in the
> > > > > > module, and
> > > > > > also respects type stub files (.pyi) when completing constructor
> > > > > > args etc.
> > > > > > In short IDEs are terrible at dynamic languages, but it is what it
> > > > > > is.
> > > > > > So a few options then:
> > > > > > Just have a deprecation warning (which it's not even clear if
> > > > > > PEP-562
> > > > > > style will show a nice (!) warning in IDEs or just at run time
> > > > > > I learn to live with from airflow.models.dag import DAG
> > > > > >
> > > > > > After we change airflow.models to not import all the sub-modules
> > > > > > automatically, go back to importing "airflow.models.dag.DAG" into
> > > > > > "airflow".
> > > > > >
> > > > > > We create a new (abstract) base class that we _can_ import in to the
> > > > > > airflow module that airflow.models.dag.DAG extends. This could be
> > > > > > automated
> > > > > > by a pre-commit so it is always up to date with the real DAG class
> > > > > >
> > > > > > We create a new module of things that are part of the public/dag
> > > > > > author
> > > > > > API, such as airflow.dag (currently just has a "BaseDAG" sub-module
> > > > > > but
> > > > > > that is barely used, and only by the scheduler so could be removed)
> > > > > >
> > > > > > Anyone have any strong opinions? I think I'd favour 3, 4 or 5
> > > > > > -ash
> > > > > > On Feb 22 2020, at 10:50 pm, Kaxil Naik <kaxiln...@gmail.com> wrote:
> > > > > > > I tried an example with PEP-562 but the autocomplete didn't work
> > > > > > > in
> > > > > >
> > > > > >
> > > > > > Pycharm
> > > > > > > and still showed the deprecated function.
> > > > > > >
> > > > > > > I agree we should have a deprecation warning before we should have
> > > > > > changed
> > > > > > > it. How about we introduce a deprecation warning in the next
> > > > > > > version
> > > > > > > (1.10.10) ?
> > > > > > >
> > > > > > > Regards,
> > > > > > > Kaxil
> > > > > > > ᐧ
> > > > > > >
> > > > > > > On Sat, Feb 22, 2020 at 5:35 PM Jarek Potiuk
> > > > > > > <jarek.pot...@polidea.com>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > I also started to use VSCode in parallel on my Chromebook as
> > > > > > > > this is
> > > > > > the
> > > > > > > > best way to get devenv running there (and I believe it's now
> > > > > > > > THE most
> > > > > > > > popular IDE - including for Python developers). I can check it
> > > > > > > > there as
> > > > > > > > well.
> > > > > > > >
> > > > > > > > On Sat, Feb 22, 2020 at 6:33 PM Jarek Potiuk
> > > > > > > > <jarek.pot...@polidea.com
> > > > > > > > wrote:
> > > > > > > > > Right -> the same. Happy to double check with your POC :)
> > > > > > > > > On Sat, Feb 22, 2020 at 6:16 PM Ash Berlin-Taylor
> > > > > > > > > <a...@apache.org>
> > > > > > > >
> > > > > > > >
> > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > That's a very good point about IDE's, I'll double check how
> > > > > > > > > > PyCharm
> > > > > > > > > behaves (Guessing PyCharm is the most popular one? PyCharm and
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > IntelliJ
> > > > > > > >
> > > > > > > > are
> > > > > > > > > the same engine under the hood, right?)
> > > > > > > > > >
> > > > > > > > > > -a
> > > > > > > > > > On Feb 22 2020, at 4:58 pm, Jarek Potiuk
> > > > > > > > > > <jarek.pot...@polidea.com
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > wrote:
> > > > > > > > > > > > My proposal here:
> > > > > > > > > > > > Add in a _getattr_ based lazy import for DAG to
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > airflow/__init__.py
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > module
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > I am all for it - if we can do it in this way, then it is
> > > > > > > > > > > indeed
> > > > > > > > > > > better for the users.
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Do NOT issue a deprecation warning for this.
> > > > > > > > > > > > Revert the change to all the imports in example dags
> > > > > > > > > > > > etc so
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > that
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > those do from airflow import DAG.
> > > > > > > > > > > I would only be for it if the PEP-562 change works fine
> > > > > > > > > > > with IDE
> > > > > > > > > > > support. Many of our users use IDEs to write their
> > > > > > > > > > > DAGsand they
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > can
> > > > > > > > > > > get confused where to import the DAGs from. If the
> > > > > > > > > > > PEP-562 works
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > fine
> > > > > > > > > > > with the IDEs, and they are smart enough to figure this
> > > > > > > > > > > out, I
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > am ok
> > > > > > > > > > > with that.
> > > > > > > > > > >
> > > > > > > > > > > On a related note - I fully agree with the "clear API"
> > > > > > > > > > > approach
> > > > > > for
> > > > > > > > > > > DAG developers. We did not have clear rules for that so
> > > > > > > > > > > far.
> > > > > > > > > > > Eventually (I think 2.0 is good time for that) we should
> > > > > > > > > > > have a
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > clear
> > > > > > > > > > > statement what really the API is for DAG writers. We
> > > > > > > > > > > should
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > issue at
> > > > > > > > > > > first a deprecation warning and then error if someone
> > > > > > > > > > > tries to
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > use
> > > > > > > > > > > something not explicitly allowed from the DAG. It should
> > > > > > > > > > > not be a
> > > > > > > > > > > convention ("from airflow.* is allowed in
> > > > > > > > > > > documentation"). It
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > should
> > > > > > > > > > > be enforcement. We can do it in scheduler - as we are
> > > > > > > > > > > parsing the
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > > DAGs
> > > > > > > > > > > on our own and we can find out what imports the users are
> > > > > > > > > > > using.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > We
> > > > > > > > > > > could even think about adding those deprecations in 2.0.
> > > > > > > > > > > There
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > are
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > > not
> > > > > > > > > > > that many things that should be importable from
> > > > > > > > > > > 'airflow.*'. And
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > that
> > > > > > > > > > > should be rather easy - I am playing now with AST parser
> > > > > > > > > > > and it
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > is
> > > > > > > > > > > rather straightforward - we just need to generate list of
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > "allowed"
> > > > > > > > > > > imports .
> > > > > > > > > > >
> > > > > > > > > > > > (I'm less worried about the internal API airflow users
> > > > > > internally,
> > > > > > > > > so airflow.models.TaskInstance vs
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > airflow.models.task_instance.TaskInstance
> > > > > > > > > is okay for me. I find the later uglier but if it doesn't
> > > > > > > > > affect
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > > users I
> > > > > > > > > don't mind it)
> > > > > > > > > > > > Ash
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > --
> > > > > > > > > > > Jarek Potiuk
> > > > > > > > > > > Polidea | Principal Software Engineer
> > > > > > > > > > >
> > > > > > > > > > > M: +48 660 796 129
> > > > > > > > > --
> > > > > > > > > Jarek Potiuk
> > > > > > > > > Polidea | Principal Software Engineer
> > > > > > > > >
> > > > > > > > > M: +48 660 796 129
> > > > > > > > --
> > > > > > > > Jarek Potiuk
> > > > > > > > Polidea <https://www.polidea.com/> | Principal Software Engineer
> > > > > > > >
> > > > > > > > M: +48 660 796 129 <+48660796129>
> > > > > > > > [image: Polidea] <https://www.polidea.com/>
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> > >
> >
> >
>
>
