Skip to content

Conversation

@AetherUnbound
Copy link
Contributor

Make sure you have checked all steps below.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:

PR #4691 introduced an issue where named tuples were inadvertently converted to a list of rendered entities. This then made subsequent attribute access on what was assumed to be a named tuple fail, since the object was actually a list.

Example:

from collections import namedtuple

Test = namedtuple('Test', ['foo', 'bar'])
t = Test(1, 2)

t = render_template(t)  # `t` is now a list of [1, 2]
print(t.foo)  # This fails after rendering

This PR adds an additional clause in the base render function to catch named tuples, and re-instantiate them as the proper class after variable rendering.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    This adds another clause to the python operator tests to ensure that rendered op_args that are named tuples are still preserved.

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.
    • 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

@AetherUnbound AetherUnbound force-pushed the bugfix/python-operator-named-tuple-template branch from 9a1ffda to 11c67a7 Compare July 28, 2019 01:54
@AetherUnbound
Copy link
Contributor Author

The kube git test is failing for what looks like a timeout, not sure how to restart it.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Would it be too much to ask you to separate out the test for template and add jinja template to the value of the named tuple in those tests (maybe alongside adding dict, list etc.).

@AetherUnbound
Copy link
Contributor Author

@potiuk I can definitely move those test cases into a separate test, as well as ensure that the named tuple values are actually rendered too. I'm happy to add those cases to any base template rendering tests that exist instead, do you know where those might be located?

@AetherUnbound
Copy link
Contributor Author

"""Tests if render_template from a field works"""

Ha, answered my own question! I'll add a test case here for named tuples.

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Your PR is failing Flake8 and mypy tests - please ensure these at least are passing locally before pushing your next commit/fixup.

@AetherUnbound AetherUnbound force-pushed the bugfix/python-operator-named-tuple-template branch 2 times, most recently from 5152301 to 35c2a62 Compare July 29, 2019 16:46
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@BasPH @potiuk Any other comments?

[AIRFLOW-4451] Fix separate checks for rendering lists & tuples

Co-Authored-By: Bas Harenslak <[email protected]>
@AetherUnbound AetherUnbound force-pushed the bugfix/python-operator-named-tuple-template branch from 72c91bc to 4dca2df Compare July 29, 2019 17:22
@BasPH
Copy link
Contributor

BasPH commented Jul 29, 2019

LGTM, we can merge once CI is successful.

@ashb
Copy link
Member

ashb commented Jul 29, 2019

======================================================================
52) FAIL: test_render_template_named_tuple_field (tests.models.test_dag.DagTest)
----------------------------------------------------------------------
   Traceback (most recent call last):
    tests/models/test_dag.py line 472 in test_render_template_named_tuple_field
      self.assertListEqual(expected, actual)
   AssertionError: First sequence is not a list: Named(var1='bar_1', var2='bar_2')
======================================================================
53) FAIL: test_render_template_tuple_field (tests.models.test_dag.DagTest)
----------------------------------------------------------------------
   Traceback (most recent call last):
    tests/models/test_dag.py line 454 in test_render_template_tuple_field
      ('bar_1', 'bar_2')
   AssertionError: First sequence is not a list: <generator object BaseOperator.render_template_from_field.<locals>.<genexpr> at 0x7fb64096f200>

@AetherUnbound AetherUnbound force-pushed the bugfix/python-operator-named-tuple-template branch from 4995a67 to 76cfeeb Compare July 29, 2019 21:59
@AetherUnbound
Copy link
Contributor Author

Tests passing! 🎉

@potiuk potiuk merged commit 6e425f4 into apache:master Jul 30, 2019
@AetherUnbound AetherUnbound deleted the bugfix/python-operator-named-tuple-template branch July 30, 2019 05:34
@AetherUnbound
Copy link
Contributor Author

Thanks all for the help! Glad we'll be able to get this in 1.10.4 😁

potiuk pushed a commit that referenced this pull request Jul 30, 2019
dharamsk pushed a commit to postmates/airflow that referenced this pull request Aug 8, 2019
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