[AIRFLOW-780] Fix dag import errors no longer working#2018
[AIRFLOW-780] Fix dag import errors no longer working#2018aoen wants to merge 1 commit intoapache:masterfrom
Conversation
aoen
force-pushed
the
fix_parse_errors_not_displa
branch
2 times, most recently
from
e806d54 to
d635384
Compare
There was a problem hiding this comment.
Good catch, I must have hit the increment number shortcut by accident! Surprisingly the test still passed.
aoen
force-pushed
the
fix_parse_errors_not_displa
branch
6 times, most recently
from
a61cb52 to
6e6ca87
Compare
There was a problem hiding this comment.
and.
if not all([s in content for s in (b'DAG', b'airflow')]):
self.file_last_changed[filepath] = file_last_changed_on_disk
return found_dags
There was a problem hiding this comment.
I think this is not common practice in sqlalchemy?
There was a problem hiding this comment.
Undid this change (it goes against the pep8 which is why my linter caught it).
aoen
force-pushed
the
fix_parse_errors_not_displa
branch
14 times, most recently
from
3944906 to
66cefe7
Compare
The import errors were no longer working after the multiprocessor update (since they are cleared after each dag directory is parsed). This change fixes them, and adds tests to prevent future regressions. Also fix Py3 incompat in BaseTaskRunner and a couple of linter errors.
aoen
force-pushed
the
fix_parse_errors_not_displa
branch
from
66cefe7 to
3e226eb
Compare
|
@bolkedebruin Addressed comments and waiting for your LGTM. Would be nice to get this for the RC since there is a regression from 1.7.1 otherwise (and it makes things harder operationally since users will ask you why their dags aren't running). |
Current coverage is 67.01% (diff: 46.15%)@@ master #2018 diff @@
==========================================
Files 138 138
Lines 10526 10531 +5
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 7028 7057 +29
+ Misses 3498 3474 -24
Partials 0 0
|
|
I'm not too happy with skipping tests on Travis as it is the only place (publicly) where all tests always run. Can you share what the Travis error was? Rest LGTM, but I do think it warrants another Beta as it touches scheduler core. |
|
I agree with you that it's not ideal. I spent about half a day debugging but it was slow-going (I even installed pyenv and used the same version of python in travis). Note that this test coverage was missing before so it's still an improvement. I tried binary searching the code with print statements but this was also slow going. The travis error was that the import errors simply weren't added in all of the tests that should have added them for python 3. When we have containers these kinds of reproducibility problems will go away. |
|
K lets put it in then |
aoen
changed the title
The import errors were no longer working after the multiprocessor update (since they are cleared after each DAG directory is parsed). This change fixes them, and adds tests to prevent future regressions. Also fix a couple of linter errors. Note that there are a few inefficiencies (e.g. sometimes we delete then add import errors in the same place instead of just doing an update), but this is equivalent to the old behavior. Testing Done: - Added missing unit tests for dag imports. Note that some of them strangely fail for python 3 and it became too time consuming to debug since I don't have a copy of the travis environment, I even ran with the same version of python locally and couldn't reproduce. I have skipped those 3 tests in python 3 for now. Closes #2018 from aoen/fix_parse_errors_not_displa
|
@bolkedebruin I merged the change and bumped the beta branch version to beta 5. |
The import errors were no longer working after the multiprocessor update (since they are cleared after each DAG directory is parsed). This change fixes them, and adds tests to prevent future regressions. Also fix a couple of linter errors. Note that there are a few inefficiencies (e.g. sometimes we delete then add import errors in the same place instead of just doing an update), but this is equivalent to the old behavior. Testing Done: - Added missing unit tests for dag imports. Note that some of them strangely fail for python 3 and it became too time consuming to debug since I don't have a copy of the travis environment, I even ran with the same version of python locally and couldn't reproduce. I have skipped those 3 tests in python 3 for now. Closes apache#2018 from aoen/fix_parse_errors_not_displa
4 participants
Please accept this PR that addresses the following issues:
The import errors were no longer working after the multiprocessor update
(since they are cleared after each DAG directory is parsed). This change
fixes them, and adds tests to prevent future regressions.
Also fix Py3 incompat in BaseTaskRunner and a couple of linter errors.
Note that there are a few inefficiencies (e.g. sometimes we delete then add import errors in the same place instead of just doing an update), but this is equivalent to the old behavior.
Testing Done:
@bolkedebruin @artwr @mistercrunch @plypaul