potiuk commented on a change in pull request #6596: [AIRFLOW-6004] Untangle
Executors class to avoid cyclic imports
URL: https://github.com/apache/airflow/pull/6596#discussion_r352941360
##########
File path: airflow/executors/dask_executor.py
##########
@@ -36,9 +35,7 @@ def __init__(self, cluster_address=None):
super().__init__(parallelism=0)
if cluster_address is None:
cluster_address = conf.get('dask', 'cluster_address')
- if not cluster_address:
- raise ValueError(
- 'Please provide a Dask cluster address in airflow.cfg')
+ assert cluster_address, 'Please provide a Dask cluster address in
airflow.cfg'
Review comment:
If we do not want to use asserts we should (now we can with pre-commits)
fail if someone adds it, not only remove them.
However I would not dismiss the asserts immediately and easily. They are so
much nicer to read in the code.
Indeed "-O" option removes the asserts but this is pretty much everything it
does (from Python docs):
> Remove assert statements and any code conditional on the value of _
_debug_ _. Augment the filename for compiled (bytecode) files by adding .opt-1
before the .pyc extension (see PEP 488). See also PYTHONOPTIMIZE.
I am not sure if we plan to use -O. However even if we use it, I think
pretty much all those asserts are really of "This should never happen" case.
Those places I removed the asserts were really the "impossible" cases that we
should never get to. Even if we do, we will fail rather sooner than later when
trying to execute a method of a None field. I am happy to review all the assert
code and see if that's the case (it was like that in my places as it was really
mypy complaining that Optional[type] has no some method when it's None. So the
worse case we will just get slightly less meaningful message requiring to take
a look at the source code.
One comment:
We do not use _ _debug_ _ anywhere in our code - I also checked all the
libraries and there are just a few places where _ _debug_ _ is used:
```
./site-packages/_pytest/assertion/rewrite.py:PYC_EXT
./site-packages/bleach/_vendor/html5lib/_inputstream.py:
./site-packages/click/_unicodefun.py:
./site-packages/click/core.py:
./site-packages/click/types.py:
./site-packages/coverage/parser.py:
./site-packages/flask_openid.py:
./site-packages/future/backports/test/support.py:
./site-packages/jinja2/nodes.py:if
./site-packages/jinja2/runtime.py:
./site-packages/mypy/newsemanal/semanal.py:
./site-packages/mypy/semanal_pass1.py:
./site-packages/pandas/core/reshape/merge.py:if
./site-packages/parso/python/errors.py:
./site-packages/pip/_vendor/distlib/_backport/misc.py:
./site-packages/pip/_vendor/distlib/compat.py:
./site-packages/pip/_vendor/html5lib/_inputstream.py:
./site-packages/py2app/util.py:
./site-packages/pygments/lexers/modula2.py:
./site-packages/pygments/lexers/nit.py:
./site-packages/werkzeug/wrappers/base_response.py:
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
