[AIRFLOW-3735] Separate dag parsing from checking #4880

ffinfo · 2019-03-08T09:08:37Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title.
- https://issues.apache.org/jira/browse/AIRFLOW-3735

Description

Extracting parsing to a separated method

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
- All the public functions and the classes in the PR contain docstrings that explain what it does
- If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

Passes flake8

airflow/models/__init__.py

ashb · 2019-03-08T16:40:31Z

If the PR is not yet ready for review on Airflow community can you hold off on opening a PR - our Travis builds are paid for by the ASF and we don't want to use more than we have to :) (I do much of my testing on my own fork with travis set up there for instance. See CONTRIBUTING.md)

airflow/models/__init__.py

tests/__init__.py

ffinfo · 2019-03-09T08:12:47Z

@ashb It is ready but once again I got this test_email_alert test that is failing on travis but not on my local machine. I did mange to fix this test on my edge branch after a lot of tries on travis. But now I tried to reproduce the steps but still it does not work :(

Any idea maybe?

About the travis thing. I can understand this but doing this on a fork still means load for travis. This issue particular because I can't reproduce it local. I try only to open a PR once I think it's done but travis seems to be very good to give unexpected errors.

…est setting like postgress/mysql/sqlite

tests/core.py

codecov-io · 2019-03-09T18:22:14Z

Codecov Report

Merging #4880 into master will increase coverage by 0.02%.
The diff coverage is 97.61%.

@@            Coverage Diff             @@
##           master    #4880      +/-   ##
==========================================
+ Coverage   76.23%   76.26%   +0.02%     
==========================================
  Files         466      466              
  Lines       30101    30107       +6     
==========================================
+ Hits        22949    22960      +11     
+ Misses       7152     7147       -5

Impacted Files	Coverage Δ
airflow/operators/subdag_operator.py	`90.62% <100%> (+0.62%)`	⬆️
airflow/models/__init__.py	`93.06% <95.45%> (+0.05%)`	⬆️
airflow/utils/dag_processing.py	`63.09% <98.33%> (+3.68%)`	⬆️
airflow/models/connection.py	`65.53% <0%> (ø)`	⬆️
airflow/models/taskfail.py	`100% <0%> (+4.54%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 463c183...0043b33. Read the comment docs.

ffinfo · 2019-03-09T19:02:46Z

tests/models/test_dagbag.py

@@ -0,0 +1,583 @@
+# -*- coding: utf-8 -*-


This is just moving code from (test_)models.py

tests/__init__.py

# Conflicts: # tests/cli/test_cli.py # tests/test_models.py

# Conflicts: # tests/models/__init__.py

Fokko · 2019-04-05T12:33:34Z

airflow/models/__init__.py

@@ -1739,7 +1669,7 @@ def email_alert(self, exception):
        jinja_context.update(dict(
            exception=exception,
            exception_html=exception_html,
-            try_number=self.try_number - 1,
+            try_number=self.try_number,


Why did we change this?

This was because the test order did change because I moved the DagBag tests to a separated file. In the testing the state of a task would be changed from RUNNING to something else. the try_number in TaskInstances does a +1 based on the state. Not the cleanest way. @ashb, I see this part is on your name. Can you double check the logic here?

About all I remember of this code now was it was a hack on top of a hack :(

Fokko · 2019-04-05T12:35:21Z

airflow/utils/dag_processing.py

@@ -1309,3 +1315,81 @@ def end(self):
                    self.log.info("Killing child PID: %s", child.pid)
                    child.kill()
                    child.wait()
+
+
+def process_dag_file(filepath, safe_mode=True):


Personally, I'm not a fan of separate functions, can't we merge this into a class as a static method? For example the DagFileProcessorAgent.

For functionality this does not matter. Adding a class that will never be initialised feels wrong for me. If this is part of a class this would make sense but this is an independent method that can be used in multiple location and not bound to a single class.

Still if this is a blocking issue I can move to an existing class or make a new class for this.

Fokko

Some small issues. As far as I see it, it is just moving a lot of stuff out of models to separate files. In this case, it is out of the infamous models.py 👍

stale · 2019-09-03T17:42:25Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ffinfo added 3 commits March 8, 2019 09:56

Add dag parse method

1d61360

Remove parse code from DagBag

ec8e858

Fixing subdag test

b419cd8

ffinfo mentioned this pull request Mar 8, 2019

[WIP][AIRFLOW-3735] Separated parsing from checking #4562

Closed

6 tasks

ffinfo added 4 commits March 8, 2019 10:47

Fixing flake8 issues

33a97d1

Fixing DagBag tests

92b07c4

fixing flake8 issues

db0220f

Fixing tests

e0199d6

BasPH reviewed Mar 8, 2019

View reviewed changes

airflow/models/__init__.py Outdated Show resolved Hide resolved

ffinfo added 3 commits March 8, 2019 16:29

Adding dag to TI

eecc158

Typo

d41eb72

Added default args

eef777b

ashb reviewed Mar 8, 2019

View reviewed changes

airflow/models/__init__.py Outdated Show resolved Hide resolved

ashb reviewed Mar 8, 2019

View reviewed changes

tests/__init__.py Outdated Show resolved Hide resolved

ffinfo added 4 commits March 9, 2019 09:23

Remove import

0ad5f37

Move corrupt files tto limit travis logs

e2d4fca

Remove test, this test is invalid because the airflow_db depends on t…

1f32259

…est setting like postgress/mysql/sqlite

Fixing test to test the real output

52feaca

ffinfo commented Mar 9, 2019

View reviewed changes

tests/core.py Outdated Show resolved Hide resolved

Make error python 2.7 compatible

aca4c08

ffinfo commented Mar 9, 2019

View reviewed changes

tests/models/test_dagbag.py

@@ -0,0 +1,583 @@

# -*- coding: utf-8 -*-

Copy link

Contributor Author

ffinfo Mar 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just moving code from (test_)models.py

ashb reviewed Mar 9, 2019

View reviewed changes

tests/__init__.py Outdated Show resolved Hide resolved

ffinfo added 2 commits March 11, 2019 12:40

Merge remote-tracking branch 'origin/master' into AIRFLOW-3735

66a15df

# Conflicts: # tests/cli/test_cli.py # tests/test_models.py

Merge remote-tracking branch 'origin/master' into AIRFLOW-3735

a25b82c

# Conflicts: # tests/models/__init__.py

Fokko reviewed Apr 5, 2019

View reviewed changes

Remove pyc exclusion

0043b33

stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 3, 2019

stale bot closed this Sep 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-3735] Separate dag parsing from checking #4880

[AIRFLOW-3735] Separate dag parsing from checking #4880

ffinfo commented Mar 8, 2019 •

edited

Loading

ashb commented Mar 8, 2019

ffinfo commented Mar 9, 2019 •

edited

Loading

codecov-io commented Mar 9, 2019 •

edited

Loading

ffinfo Mar 9, 2019

Fokko Apr 5, 2019

ffinfo Apr 5, 2019

ashb Apr 5, 2019

Fokko Apr 5, 2019

ffinfo Apr 5, 2019

Fokko left a comment

stale bot commented Sep 3, 2019

[AIRFLOW-3735] Separate dag parsing from checking #4880

[AIRFLOW-3735] Separate dag parsing from checking #4880

Conversation

ffinfo commented Mar 8, 2019 • edited Loading

Jira

Description

Tests

Commits

Documentation

Code Quality

ashb commented Mar 8, 2019

ffinfo commented Mar 9, 2019 • edited Loading

codecov-io commented Mar 9, 2019 • edited Loading

Codecov Report

ffinfo Mar 9, 2019

Choose a reason for hiding this comment

Fokko Apr 5, 2019

Choose a reason for hiding this comment

ffinfo Apr 5, 2019

Choose a reason for hiding this comment

ashb Apr 5, 2019

Choose a reason for hiding this comment

Fokko Apr 5, 2019

Choose a reason for hiding this comment

ffinfo Apr 5, 2019

Choose a reason for hiding this comment

Fokko left a comment

Choose a reason for hiding this comment

stale bot commented Sep 3, 2019

ffinfo commented Mar 8, 2019 •

edited

Loading

ffinfo commented Mar 9, 2019 •

edited

Loading

codecov-io commented Mar 9, 2019 •

edited

Loading