Skip to content

[WIP] force jinja to preserve native types #23943

Closed
jctanner wants to merge 11 commits intoansible:develfrom
jctanner:test_jinja_hack
Closed

[WIP] force jinja to preserve native types #23943
jctanner wants to merge 11 commits intoansible:develfrom
jctanner:test_jinja_hack

Conversation

@jctanner
Copy link
Copy Markdown
Contributor

@jctanner jctanner commented Apr 25, 2017

Subclasses the CodeGenerator class and refactors visit_Output to exclude the default to_string() class it injects.

SUMMARY

Subclasses the CodeGenerator class and refactors visit_Output to exclude the default to_string() class it injects.

Upstream effort: pallets/jinja#708

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

templar

ANSIBLE VERSION
2.4

@jctanner jctanner changed the title force jinja to preserve native types [WIP] force jinja to preserve native types Apr 25, 2017
@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.4 This issue/PR affects Ansible v2.4 feature_pull_request needs_triage Needs a first human triage before being processed. labels Apr 25, 2017
Comment thread lib/ansible/template/__init__.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

s/stry,bytes,unicode/string_types,text_types/

@ryansb ryansb removed the needs_triage Needs a first human triage before being processed. label Apr 25, 2017
bcoca
bcoca previously requested changes Apr 25, 2017
Comment thread lib/ansible/template/__init__.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is not compatible across python versions use text_strings, byte_strings from our 'compatiblity' code instead

Comment thread lib/ansible/template/__init__.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

generator, not gnerator

Comment thread lib/ansible/template/__init__.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Camel case and underscores? :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sometimes you have to allow for all kinds ...

elif len(invals) > 1:
# cast to unicode and join
invals = u''.join([u'%s' % x for x in invals])
return invals
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could make this more efficient. what types can be given here? Since the original jinja2 code is u"".join(invals) it seems like the minimum assumptions we can make are that invals is an iterable and that all the elements can convert to a text type without unicode exceptions. So maybe something like this:

# We can omit this block if invals is never a string type
if isinstance(invals, six.text_type):
    return invals
elif isinstance(invals, six.binary_type):
    return to_text(invals, errors='surrogate_or_strict')

try:
    if len(invals) == 1:
        # Break out the single value
        return invals[0]
except TypeError:
    # Iterable (by contract) but not a Sequence
    pass

return u''.join(invals)
  • This keeps us from instantiating two lists.
  • This should be faster even than the jinja2 concat if strings (not inside of a container) are allowed as invals.
  • If we don't have to deal with strings, then we can get rid of that block and reduce the number of conditionals.

I can adapt that further if we need to make less assumptions (or if we can make more of them).

Copy link
Copy Markdown
Contributor

@abadger abadger Apr 27, 2017

Choose a reason for hiding this comment

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

Okay, jctanner says that this is always a generator. So this is a better implementation:

from itertools import chain
start = []
for val in invals:
    start.append(val)
    if len(start) > 1:
        break
else:
    if start:
        return start[0]
    return []      
return u''.join(chain(start, invals))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Profiling and jctanner's version is faster. So the only things I can see to do differently:

  • remove "if isinstance(invals, list):". after the list comprehension, invals should always be a list.
  • u'%s' % x for x in invals note that this isn't 100% safe (for instance b'café' will raise a unicode error) but I assume you took it from some part of jinja2 code so we aren't making a new assumption then.

@jctanner
Copy link
Copy Markdown
Contributor Author

Another usecase that came up on IRC today ...

  vars:
    A: '{{ a_node | default(False) }}'
    B: '{{ b_node | default(True) }}'

A and B will preserve their bool types in that case with this patch and no downstream consumers would have to guess at the intent.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 30, 2017
@bcoca
Copy link
Copy Markdown
Member

bcoca commented May 30, 2017

another example of same boolean:

  vars:
    A: '{{ a_node is defined }}'
    B: '{{ b_node is undefined }}'

@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 30, 2017
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jun 23, 2017
@ansibot ansibot added the test This PR relates to tests. label Sep 1, 2017
@cognifloyd
Copy link
Copy Markdown
Contributor

I'm happy to see that the upstream changes have landed and will be in 2.10. 👍

Any chance this will make it in to Ansible before then to make this feature available when using, say, Jinja 2.9? Or is the plan to wait until 2.10 gets released, and use that?

@halberom
Copy link
Copy Markdown

halberom commented Nov 6, 2017

@cognifloyd, I commented in #ansible-devel with much the same query, and jtanner responded with

12:50 <@jtanner> we can vendor our own copy of the relevant code until it's in mainstream
12:50 <@jtanner> https://github.com/ansible/ansible/pull/23943

@ansible ansible deleted a comment from ansibot Nov 6, 2017
@ansible ansible deleted a comment from ansibot Nov 6, 2017
@ansible ansible deleted a comment from ansibot Nov 6, 2017
@ansible ansible deleted a comment from ansibot Nov 6, 2017
@ansible ansible deleted a comment from ansibot Nov 6, 2017
@cognifloyd
Copy link
Copy Markdown
Contributor

Sorry about mis-posting this message moments ago.

Jinja 2.10 is out. What is involved in increasing ansible's version requirements? Is that something that can take a year or two or more? What makes the release "mainstream"? RPM releases in RHEL or packages in other distros or ...?
https://pypi.python.org/pypi/Jinja2/2.10
http://jinja.pocoo.org/docs/2.10/nativetypes/

@jctanner
Copy link
Copy Markdown
Contributor Author

jctanner commented Nov 9, 2017

New PR #32738

@jctanner jctanner closed this Nov 9, 2017
@ansibot ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 4, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

affects_2.4 This issue/PR affects Ansible v2.4 feature This issue/PR relates to a feature request. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants