Good to hear :) Le mar. 5 juil. 2022 à 21:26, Ash Berlin-Taylor <[email protected]> a écrit :
> Oh And it works *really* nicely: It places it in the right order (very > first import) and it doesn't add it to "empty" __init__.py files. Just what > we want. > > On Tue, Jul 5 2022 at 20:22:48 +0100, Ash Berlin-Taylor <[email protected]> > wrote: > > Oh not quite -- it will remove any unused imports, but I guess Pierre was > suggesting using isort to add the future annotation line. Gotcha. > > -a > > On Tue, Jul 5 2022 at 20:21:14 +0100, Ash Berlin-Taylor <[email protected]> > wrote: > > We've already got this in our pre-commit config that does the same (at > least from what I can tell from a quick test) > > ``` > - id: static-check-autoflake > name: Remove all unused code > entry: autoflake --remove-all-unused-imports > --ignore-init-module-imports --in-place > language: python > additional_dependencies: ['autoflake'] > ``` > > Added by one Jarek Potiuk https://github.com/apache/airflow/pull/20466 :D > > > On Tue, Jul 5 2022 at 19:03:45 +0200, Jarek Potiuk <[email protected]> > wrote: > > O wow. nice feature of isort :) TIL. > > On Tue, Jul 5, 2022 at 7:01 PM Jeambrun Pierre <[email protected]> > wrote: > >> Hello, >> >> I also like the postponed annotation evaluation. Getting rid of most >> forward references is by itself a strong motivation for me, getting >> performance improvement on top of that is also great. >> >> Regarding concerns about PEP 649 superseding PEP 543, I do also agree >> that this change would not be this complicated to make. The SC is even >> considering rewiring the `from __future__ import annotations` to enable pep >> 649 in case it's favored over 543. >> >> *isort* can enforce automatic imports addition or deletion via the >> *add_imports* and *remove_imports* options. (might be good to check if >> this can help achieve what we want): >> https://pycqa.github.io/isort/docs/configuration/options.html >> >> Best, >> Pierre >> >> Le lun. 4 juil. 2022 à 20:06, Jarek Potiuk <[email protected]> a écrit : >> >>> Yeah. I certainly do not want to merge it now in the form it is (also TP >>> mentioned that in the comment that in this form it is impossible to >>> review). This is more POC but it can be done 'better' - for example the >>> empty __init__.py do not have to have the __tuture__ annotations added. I >>> think - if the feedback will be general "we like it" then we can also >>> introduce it in stages (folder-by-folder) to get it reviewable. >>> >>> J. >>> >>> On Mon, Jul 4, 2022 at 5:54 PM Ash Berlin-Taylor <[email protected]> wrote: >>> >>>> I certainly like the future-annotations and have idly thought about how >>>> to do this myself. >>>> >>>> The only downside I can think of is that it might makes cherry picks >>>> back to the 2.3 release branch more complicated in some cases (mostly >>>> because we generally try to avoid backporting things that aren't bug >>>> fixes.) >>>> >>>> So maybe we merge this in the prep stage for 2.4 instead of right now? >>>> >>>> -ash >>>> >>>> On Mon, Jul 4 2022 at 00:12:41 +0200, Jarek Potiuk <[email protected]> >>>> wrote: >>>> >>>> Hey everyone, >>>> >>>> *TL;DR;* I wanted to discuss that maybe (not really sure- but I wanted >>>> to check with others) we could migrate the whole codebase of airflow to >>>> support PEP 563 - postponed evaluation of annotations. PEP 563 is nicely >>>> supported in our case (Python 3.7+) by `from __future__ import annotations` >>>> and pyupgrade automated pre-commit does the bulk of the work of converting >>>> our code automatically - both current and future. >>>> >>>> *Is there a POC?* >>>> >>>> Yeah. I have a PR that implements it. I actually implemented it today - >>>> literally in a few hours, in-between of other things. >>>> >>>> * On one hand it is huge= 3625 files (+16,459 −11,989 lines) >>>> * On the other hand it really only **really** changes type annotations >>>> (so no real code impact) and those changes are semi-automated. I >>>> literally got it "generated" in a couple of hours - and I can easily either >>>> rebase or reapply it on top of new changes. >>>> >>>> There are some spelling mistakes to fix and few tests to fix, but this >>>> should be easy. >>>> >>>> PR here: https://github.com/apache/airflow/pull/24816 >>>> >>>> *Why ?* >>>> >>>> Our code gets more and more type-annotated and it's cool. But it does >>>> not come for free (for now). The types for functions/classes are evaluated >>>> at import time and it takes some computational effort. Also we have a >>>> number of cases where we have "string" forward references. This is ugly and >>>> does not get as good support for auto-completions and IDEs in general. >>>> >>>> PEP 563 solves both problems. There is an alternative PEP 649 that >>>> could be chosen in the future, but I think we can get some benefits now >>>> without waiting for a decision (which even if it comes, will be only >>>> implemented in 3.11 or maybe in 3.12 and in the meantime we could get some >>>> benefits of PEP 563). >>>> >>>> 1) Runtime performance is improved in case we do not use type-hints at >>>> runtime (we don't) - static type checkers work as usual, but at runtime >>>> those annotations are placed in _annotations_ dictionary and evaluated only >>>> when runtime type-hint checking is used. >>>> >>>> 2) String forward references are not needed. >>>> No longer `-> 'Type' when you define the type a few lines above, simple >>>> `-> Type` will do. >>>> >>>> Those two on its own would be worth it IMHO, but there is more. >>>> >>>> 3) the new syntax of types from Python 3.10 is available and it's quite >>>> a bit nicer when it comes to Optional usage. Example: >>>> >>>> CLIENT_AUTH: Optional[Union[Tuple[str, str], Any]] = None >>>> >>>> turns into: >>>> >>>> CLIENT_AUTH: tuple[str, str] | Any | None = None >>>> >>>> 4) Interestingly enough - we do not need to do anything to use the new >>>> syntax. The pyupgrade which we have in our pre-commit will automatically >>>> convert the "old way" of using Optionals to the new one - as soon as you >>>> add `from __future__ import annotations`. This also means that if anyone >>>> uses the "old way" in the future, pre-commit will catch it and migrate - >>>> this is actually a cool learning experience for all contributors I think as >>>> they can learn and get usd to Python 3.10 syntax today. This conversion is >>>> done in my PR, you can take a look. >>>> >>>> 5) It is overwhelmingly mostly backwards-compatible for Python 3.7 >>>> except if you used runtime type annotation checking and analysis (which we >>>> did not) except inferring multiple outputs in decorators (literally single >>>> usage in the entire codebase). And the way we use it seems to be able to >>>> continue to work after we add some small fixes - I think just our tests >>>> need to be fixed (I think 9 of the failing tests to fix are falling into >>>> this camp). >>>> >>>> *Caveats* >>>> >>>> Almost none since we are already Python 3.7+. Basically each of our >>>> python files must start with: >>>> >>>> from __future__ import annotations >>>> >>>> Performance gets improved for imports. The PEP >>>> https://peps.python.org/pep-0563/ claims that the overhead for the >>>> runtime typing information needed is very small (memory wise) and since we >>>> are almost not using runtime type-hint evaluation memory usage will be >>>> lower because the type hints will not be evaluated at all during runtime. >>>> >>>> And similarly, like we do not care about licenses any more (because >>>> they are added automatically) by pre-commit, it could be the same with the >>>> __future__ import. >>>> >>>> PEP 649: >>>> >>>> The caveat is that https://peps.python.org/pep-0649/ - alternative >>>> approach to deferring annotation is still being discussed and PEP 563 is >>>> not confirmed.. PEP 649 is the reason why PEP 563 wasr removed from Python >>>> 3.10 - because there is no decision which one will go forward in the >>>> future. Discussion here: >>>> https://discuss.python.org/t/type-annotations-pep-649-and-pep-563/11363 >>>> >>>> However, one could think we are not really affected too much and even >>>> if we have to change for 3.11 it will be as simple. We do not use some >>>> heavy runtime type-hinting checks, and PEP 0649 `from __future__ import >>>> co_annotations' seems to be an easy switch. >>>> >>>> *How complex is it to migrate* >>>> >>>> It is simple. We Just merge >>>> https://github.com/apache/airflow/pull/24816 and maybe add a >>>> pre-commit to force the __future__ import in all our files. That's it. >>>> >>>> I did it in a couple of hours and I think it is all that we need - the >>>> PR I did could likely be merged today and it will be complete. Adding the >>>> "__future__" import is very easily automate-able and `pyupgrade` does all >>>> the rest. There is one exclusion that we have to make with >>>> pyupgrade (gcs.py has "list()" method that interferes with list Type). >>>> >>>> There were also a few type fixes that had to be done - but >>>> nothing major. >>>> >>>> That's about it. No drama. Some tests/docs need fixing but looking at >>>> the changes - very few. We can add pre-commits guarding it. >>>> >>>> *Options* >>>> >>>> In the PR I added the 'from __future__ import annotations` to all files >>>> (semi-automatically). But it does not have to be like that. One can think >>>> that this introduces some "noise". There are plenty of files that do not >>>> need it currently. >>>> >>>> On the other hand there is a nice "property" of having it in all the >>>> files: >>>> >>>> 1) We can even add it in the pre-commit to be injected automatically >>>> (should be simple). >>>> 2) Pyupgrade will automatically upgrade any future "forward" refs and >>>> optionals for us >>>> 3) we can keep consistent style of typing everywhere and remove >>>> __futures__ when we get to Python 3.10 >>>> >>>> But we can choose to do half-the way - for example do not add >>>> __future__ in empty __init__.py files. Or something similar. >>>> >>>> I am not 100% convinced myself, but maybe >>>> >>>> WDYT? Anyone has experience with it? bad/good one? >>>> >>>> J. >>>> >>>> >>>> >>>> >>>>
