And to be clear: I'd rather we didn't make everything an implicit namespace package, but only the "official" extension points of airflow.providers. I cant immediately think of a reason to make anything else a namespace package, and limiting this to a single place makes the change smaller (tiny even?) and also easier to reason about where modules might be coming from.
What have I not thought about? On Feb 5 2020, at 10:31 am, Ash Berlin-Taylor <[email protected]> wrote: > Are you talking about/test with making _ALL_ packages implicit namespaces? > > I would think that we would only make airflow.providers an implicit package, > but leave all the sub packages as explicit packages: i.e. this: > airflow/__init__.py > airflow/utils/__init__.py > airflow/providers/ (no __init__.py) > airflow/providers/google/__init__.py > > Does limiting the implicit namespace to just airflow.providers address 1+2+3? > -a > On Feb 5 2020, at 10:27 am, Jarek Potiuk <[email protected]> wrote: > > TL;DR; I am asking for opinions on some of the changes required to > > introduce implicit native package support. This is the easiest way to make > > backports of master providers packages to 1.10.x. > > > > > > NOTE!: @Ash Berlin-Taylor (mailto:[email protected]) @Fokko Driesprong > > (mailto:[email protected]) @Felix Uellendall (mailto:[email protected]) > > @Kamil Breguła (mailto:[email protected]) @Kaxil Naik > > (mailto:[email protected]) @[email protected] > > (mailto:[email protected]) @Philippe Gagnon > > (mailto:[email protected]) - there is one point 1a) below that I would > > like to get your input. > > > > As AIP-21 (import paths) import path move is done we have an important step > > to complete - we should switch to implicit "native" packages. It boils down > > to removal of empty (only with comment licences) __init__.py files and > > relying on python 3.3+ capability of finding packages without having to add > > __init_.py files. This is needed in order to get backported packaged to be > > prepared for 1.10.* series. I worked on it during the last few days - to > > make sure this can be done and I am close to have it. Close enough to know > > I can solve all the remaining problems. I wanted to check that we can do it > > for almost all the packages. I already solved most of the initial problems > > but I have some places where I think I need community opinion on the way we > > should solve them: > > > > The PR that has the changes is here: > > https://github.com/apache/airflow/pull/7279/files - it's not 100% ready yet > > and I want to split it into a few separate PRs so do not comment it there > > yet - I extracted the most important changes here and wanted to ask your > > opinion where I have doubts: > > > > 1) Module names identical to some top-level package names (for example > > email.py): > > > > Some of the module names (for example email.py) has to be changed. The > > problem with implicit packages is that if local module is named the same as > > the top level package, importing the top-level package from it does not > > work: > > > > email.py: > > ... > > import email <- this won't work - module imports itself. > > > > For now in my PR i renamed the modules to be airflow_<old_name>.py or > > <old_name>_utils.py : There are such modules (mostly hooks): > > email -> email_utils, (utils) > > > > cassandra -> airflow_cassandra > > > > cloudant -> airflow_cloudant > > > > dataproc -> airflow_dataproc > > > > grpc -> airlfow_grpc > > > > hdfs -> airflow_hdfs > > > > jira -> airflow_jira > > > > kerberos -> kerberos_security > > > > redis -> airflow_redis > > > > sendgrid -> airflow_sendgrid > > > > snowflake -> airflow_snowflake > > > > winrm -> airflow_winrm (operator) > > > > datadog -> airflow_datatog(sensor) > > > > sqlalchemy -> sqlalchemy_utils (utils) > > > > > > > > I can also change it to grpc_hooks.py, datadog_sensors.py etc. Or maybe > > someone knows an easy way how to keep the module name and implicit packages? > > > > I guess this is not a big problem to change it this way - we anyhow have > > changed import paths for those. The only two problems with this are: > > a) email was used in "email_backend" configuration : email_backend = > > airflow.utils.email.send_email_smtp > > but we can solve it by handling that as special case, raising deprecation > > warning and converting it to airflow.utils.email_utils.send_email_smtp > > b) we introduce slight inconsistency in hook/operator/sensor names. > > > > 2) Mypy fails when checking individual (duplicated) module names > > > > Mypy does not work well with duplicate module names in implicit packages > > (https://github.com/pre-commit/mirrors-mypy/issues/5) and with AIP-21 we > > have plenty of them. Repeating modules (http.py in both operators and hooks > > for example) cause it to fail in case they are provided as parameters (in > > case of incremental mypy check in pre-commits. I had to change it then so > > that mypy always check 'airflow' and 'test' packages instead - which makes > > pre-commit check slightly slower (few seconds). > > > > 3) _hooks.py/_operators.py/_sensors.py > > (http://hooks.py/_operators.py/_sensors.py) suffix: Some thought resulting > > from above 1) and 2): > > > > I did not want to open pandora's box again but I think removal of _hooks, > > _operators, _sensors from the module name might not have been the best > > decision. While it removes some duplication in module name, it actually > > introduces duplicate module names and (as it is with mypy) it might be a > > problem in the future. I think now we should change it back to add the > > suffixes. If we want to reverse it - this is the last moment we can do it > > (and we still can easily). I would love some quick opinion/voting for that > > - especially those who voted in AIP-21 but others are welcome as well: @Ash > > Berlin-Taylor (mailto:[email protected]) @Fokko Driesprong > > (mailto:[email protected]) @Felix Uellendall (mailto:[email protected]) > > @Kamil Breguła (mailto:[email protected]) @Kaxil Naik > > (mailto:[email protected]) @[email protected] > > (mailto:[email protected]) @Philippe Gagnon > > (mailto:[email protected]) - this was "Case 2" in the original AIP-21 > > proposal. > > > > WDYT? > > > > 4) Test module names: > > > > I had to change all test module name for providers. For example: . Pytest > > does not like repeating implicit test module names. It throws error > > "duplicate test module). For example: > > tests/providers/http/hooks/test_http.py → > > tests/providers/http/hooks/test_http_hooks.py > > (https://github.com/apache/airflow/pull/7279/files#diff-593e762ce1d79c250426a27b6fb28908) > > > > > > (basically I added hooks/operators/sensors everywhere). It is easy and > > fully automated and it has no impact at all on the main code base, so I > > think this is a super-safe change. t The nice thing is that it makes it > > easier to understand what kind of tests you are looking at immediately and. > > That also hints that maybe _hooks.py, _operators.py, _sensors.py in > > hooks/operators/sensors could have been a better choice (see 2 a) ) > > > > 5) Doc generation > > > > Autoapi doc generation has not yet released implicit package support (it is > > in master only). However I managed to monkey-patch 1.2.1 (latest) version > > to add support by cherry-picking the changes (there are literally few > > methods changed there - > > https://github.com/readthedocs/sphinx-autoapi/pull/178) and when new > > version is released we will be able to get rid of monkey-patching . I am > > still resolving a few import issues in that newest version but I am > > confident I can fix or workaround all issues with it. > > I don't expect this to be a problem - it is only in the doc generation > > code, not the main application code. > > > > I verified that pytest test discovery works fine, and that airflow can be > > installed and works with implicit packages. Let me know what you think > > about it and I would love to merge it as soon as possible (rebasing this > > for a long time might be painful). > > > > J. > > > > > > > > > > > > On Sat, Nov 23, 2019 at 9:22 AM Jarek Potiuk <[email protected] > > (mailto:[email protected])> wrote: > > > I've read a bit more about PEP-0420 so some more extracted information: > > > > > > - implicit namespace packages are very similar to regular packages. The > > > notable difference is that they can be loaded from several directories > > > (so you can have modules in the same package but coming from different > > > physical directories), so no __file__ is defined for the package (it is > > > still defined for modules), > > > > > > - pytest should have no problems. It has indeed > > > (https://stackoverflow.com/questions/50174130/how-do-i-pytest-a-project-using-pep-420-namespace-packages) > > > problems with implicit packages when tests are co-located with their > > > tested modules (module.py, module_test.py) but we are not in this > > > situation. > > > > > > - I do not see how it could be slower - I have not found any benchmarking > > > yet (but I have not found any complaints about it either). I looked at > > > the loading algorithm for PEP-420 - it traverses all the directory > > > structure available on the PYTHONPATH bo so does the "standard" > > > mechanism. And we would not have to parse and load all the 140 > > > licence-only __init__.py. The presence of module in the package is based > > > on the presence of "<module>.py" in the right folder. If we would have > > > several packages elsewhere that might become a bit slower, but I think > > > reading files is so much slower than scanning directories that - if > > > anything - we will be faster (just opening and closing those > > > __init__.files will take quite some time :). We can do some benchmarking > > > before and after to be sure. > > > > > > Side comment: I am not 100% sure, but after reading PEP-420 intuition > > > tells me that maybe implicit packages can help in solving relative > > > imports problems that some of our user raised . I saw recently several > > > users had problems with relative imports used in DAGs. We are loading the > > > DAGs in a specific way and in implicit packages, the __repr__ of modules > > > parent is dynamic - based on the location of the file rather than based > > > on the module of the file, so it's likely if we encourage dag developers > > > also to use implicit packages, it might actually solve the relative > > > imports case. > > > > > > J. > > > On Fri, Nov 22, 2019 at 4:29 PM Daniel Imberman > > > <[email protected] (mailto:[email protected])> wrote: > > > > Fine by me if it doesn’t break anything. I’m always on the side of less > > > > code :). > > > > > > > > via Newton Mail > > > > [https://cloudmagic.com/k/d/mailapp?ct=dx&cv=10.0.32&pv=10.14.5&source=email_footer_2] > > > > On Fri, Nov 22, 2019 at 8:12 AM, Ash Berlin-Taylor <[email protected] > > > > (mailto:[email protected])> wrote: > > > > Probably worth it, but things to check: > > > > > > > > - if nose or pytest needs them under tests/ to find all the tests > > > > - if not having the init files slows down pythons package importer as > > > > it searches more paths? > > > > > > > > -a > > > > > On 22 Nov 2019, at 12:06, Jarek Potiuk <[email protected] > > > > > (mailto:[email protected])> wrote: > > > > > > > > > > There is one implementation detail in AIP-21 that I continue to have > > > > > questions about, and I wanted to ask community about it. > > > > > > > > > > Since we moved to Python 3, we have the option of using implicit > > > > > namespace > > > > > packages. > > > > > https://www.python.org/dev/peps/pep-0420/ . We have now grouping per > > > > > folder > > > > > for services, so it would require a lot more __init__.py files if we > > > > > continue using them. > > > > > > > > > > The implicit naming boils down to not requiring __init__.py files if > > > > > no > > > > > initialisation of package is needed. We could do it consistently for > > > > > all > > > > > code which is "internal" to airflow. This means that most packages > > > > > will not > > > > > have __init__.py there will be few exceptions like 'airflow', > > > > > 'airflow.modules' and likely few others with the __init__.py. > > > > > > > > > > I did a quick check and we have only 25 _init_.py with some logic - > > > > > remaining ~ 140 could be removed as they only contain comments. > > > > > > > > > > find . -name '__init__.py' | xargs grep -le '^[^#].*' | wc -l > > > > > 25 > > > > > find . -name '__init__.py' |wc -l > > > > > 164 > > > > > > > > > > Those are the files that will be left: > > > > > ./tests/__init__.py > > > > > ./tests/utils/log/elasticmock/__init__.py > > > > > ./tests/utils/log/elasticmock/utilities/__init__.py > > > > > ./tests/models/__init__.py > > > > > ./tests/task/__init__.py > > > > > ./airflow/sensors/__init__.py > > > > > ./airflow/operators/__init__.py > > > > > ./airflow/_vendor/nvd3/__init__.py > > > > > ./airflow/_vendor/slugify/__init__.py > > > > > ./airflow/serialization/__init__.py > > > > > ./airflow/__init__.py > > > > > ./airflow/models/__init__.py > > > > > ./airflow/www/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/__init__.py > > > > > ./airflow/ti_deps/deps/__init__.py > > > > > ./airflow/macros/__init__.py > > > > > ./airflow/executors/__init__.py > > > > > ./airflow/lineage/__init__.py > > > > > ./airflow/lineage/backend/__init__.py > > > > > ./airflow/lineage/backend/atlas/__init__.py > > > > > ./airflow/hooks/__init__.py > > > > > ./airflow/task/task_runner/__init__.py > > > > > ./airflow/api/__init__.py > > > > > ./airflow/api/common/experimental/__init__.py > > > > > ./airflow/api/client/__init__.py > > > > > ./airflow/jobs/__init__.py > > > > > > > > > > WDYT? > > > > > > > > > > J > > > > > > > > > > -- > > > > > > > > > > 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 (tel:+48660796129) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Jarek Potiuk > > Polidea (https://www.polidea.com/) | Principal Software Engineer > > > > > > > > > > > > > > > > > > M: +48 660 796 129 (tel:+48660796129) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
