Cool! On Mon, Feb 24, 2020 at 3:58 PM Ash Berlin-Taylor <a...@apache.org> wrote:
> https://github.com/apache/airflow/pull/7517 has been merged now, so we > have kept airflow.DAG working, just lazily loaded. > > -a > On Feb 23 2020, at 10:53 pm, Kaxil Naik <kaxiln...@gmail.com> wrote: > > Yay !! Nice suggestion Kamil, good work Ash > > ᐧ > > > > On Sun, Feb 23, 2020 at 10:51 PM Ash Berlin-Taylor <a...@apache.org> > wrote: > > > 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/> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- Jarek Potiuk Polidea <https://www.polidea.com/> | Principal Software Engineer M: +48 660 796 129 <+48660796129> [image: Polidea] <https://www.polidea.com/>
