Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIRFLOW-5210] Make finding template files more efficient #5815

Merged
merged 1 commit into from
Aug 14, 2019
Merged

[AIRFLOW-5210] Make finding template files more efficient #5815

merged 1 commit into from
Aug 14, 2019

Conversation

danfrankj
Copy link
Contributor

@danfrankj danfrankj commented Aug 14, 2019

Jira

https://issues.apache.org/jira/browse/AIRFLOW-5210

Description

For large DAGs (1000s of tasks) iterating over template_fields adds significant time to task execution and is not necessary for tasks that do not specify template_ext

Tests

I can add tests if you think necessary, but this is a very simple change

Code Quality

  • Passes flake8

For large DAGs, iterating over template fields to find template files can be time intensive.
Save this time for tasks that do not specify a template file extension.
@kaxil kaxil requested a review from BasPH August 14, 2019 09:36
content[i] = env.loader.get_source(env, content[i])[0]
except Exception as e:
self.log.exception(e)
if self.template_ext:
Copy link
Member

@kaxil kaxil Aug 14, 2019

Choose a reason for hiding this comment

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

Wouldn't this stop rendering of template fields ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The way I read it, this method iterates over all template_fields and if it finds a field value with an extension listed in template_exts, runs env.loader.get_source(...) and sets the result on the specific field.

It indeed scans over all template_fields even if template_exts is empty, which doesn't make sense so I think this is a valid change.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, I get it

@BasPH
Copy link
Contributor

BasPH commented Aug 14, 2019

@danfrankj LGTM, the k8s CI step failed. I believe it's a bit buggy so restarted it.

@BasPH BasPH merged commit eeac823 into apache:master Aug 14, 2019
@BasPH
Copy link
Contributor

BasPH commented Aug 14, 2019

Thanks @danfrankj

@danfrankj
Copy link
Contributor Author

You're welcome! Thank you guys. Happy DAG'n

@danfrankj
Copy link
Contributor Author

@BasPH was something wrong with this PR? - I'm seeing a message above about a revert

@BasPH
Copy link
Contributor

BasPH commented Aug 15, 2019

No, my bad, I accidentally clicked the revert button. I deleted the "revert" branch.

ashb pushed a commit to ashb/airflow that referenced this pull request Sep 18, 2019
For large DAGs, iterating over template fields to find template files can be time intensive.
Save this time for tasks that do not specify a template file extension.

(cherry picked from commit eeac823)
ashb pushed a commit that referenced this pull request Sep 24, 2019
For large DAGs, iterating over template fields to find template files can be time intensive.
Save this time for tasks that do not specify a template file extension.

(cherry picked from commit eeac823)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants