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

Add support for the Environment to optionally return native types. #708

Merged
merged 5 commits into from Oct 31, 2017

Conversation

@jctanner
Copy link
Contributor

@jctanner jctanner commented Apr 27, 2017

This works by having an alternate CodeGenerator that avoids doing to_string
after the yield statement and a new version of concat that handles the returned
generator with a bit more "intelligence".

Related to ansible/ansible#23943

We use jinja heavily in the ansible project. Although it seems to target a text based destination for the renderers, our users have a desire to preserve the types of their templated vars. We also do a lot of internal intercept and post-processing to preserve those types but it's hit or miss and never obvious to the end user what will work and what won't. Therefore, I'm trying to extend jinja beyond what it's original usecase might have been.

This is a first pass and I hope to drive discussion with it and shape it into something the rest of the jinja community is happy with.

try:
native_tmpl.render()
failed = False
except UndefinedError:

This comment has been minimized.

@davidism

davidism May 1, 2017
Member

Should use pytest.raises.

element.
'''
invals = [x for x in invals]
if isinstance(invals, list):

This comment has been minimized.

@davidism

davidism May 1, 2017
Member

This will always be True because of the previous line.

This comment has been minimized.

@jctanner

jctanner May 1, 2017
Author Contributor

Fixed

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

This comment has been minimized.

@davidism

davidism May 1, 2017
Member

Implicit encoding seems dangerous, should use text_type(x, 'utf8') here.

This comment has been minimized.

@jctanner

jctanner May 1, 2017
Author Contributor

Fixed

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

This comment has been minimized.

@davidism

davidism May 1, 2017
Member

Why do you convert multiple nodes to a string? Wouldn't you want to return the list?

@davidism
Copy link
Member

@davidism davidism commented May 1, 2017

It took me a while to figure out what this was actually doing. Definitely needs some docs, but looks interesting.

The term "native" is a bit confusing, since I also associate it with discussion about what Python's str is in 2 vs 3.

How is Ansible using this? That is, when do users need access to the results of a render and don't have access to the original objects instead?

Does this need to be in Jinja, as opposed to a separate package providing a different Environment? Admittedly, there would be a little code duplication since the native checks happen in the middle of a couple methods. It just seems so radically different than what Jinja usually does.

@jctanner
Copy link
Contributor Author

@jctanner jctanner commented May 1, 2017

Hey @davidism

I'm working on addressing your inline code comments at the moment.

In terms of how Ansible will be using this... It's going to be transparent to our users, except that when they have chained template operations inside a playbook, types will be preserved all the way through.

Here's a basic example:

- hosts: el6host
  connection: local
  gather_facts: False
  vars:
      adict:
          foo: "1"
  tasks:
    - set_fact:
        tempres: "{{ adict.foo|int + 3 }}"
    - debug: var=tempres
    - debug: msg="{{ tempres | type_debug }}"

The type for "tempres" is -always- unicode in Ansible right now. With this feature, it will be an integer.

elif len(invals) > 1:
# cast to unicode and join
try:
invals = u''.join([text_type(x, 'utf8') for x in invals])

This comment has been minimized.

@davidism

davidism May 1, 2017
Member

Never mind, I messed up the review for this one. Should use text_type, which is what runtime.to_string is. But don't specify an encoding, since that's only relevant if the object is bytes. Should remove the except Exception block.

invals = u''.join([text_type(x, 'utf8') for x in invals])
except Exception as e:
pass
return invals

This comment has been minimized.

@davidism

davidism May 1, 2017
Member

Should this be return None? An empty list doesn't make much sense.

This comment has been minimized.

@davidism

davidism May 1, 2017
Member

Also need a test case for this path.

native, the list is artificial and we should return just the first
element.
'''
invals = [x for x in invals]

This comment has been minimized.

@davidism

davidism May 1, 2017
Member

I don't like building this list, seems wasteful of memory, although I guess there shouldn't be many nodes for the use case you're solving. Could change this to head = list(islice(invals, 2)) to test the length, then either return head[0] or build a list from invals in join.

self.native = native
if self.native:
self.code_generator_class = NativeCodeGenerator
#import pytest; pytest.set_trace()

This comment has been minimized.

@@ -335,6 +336,11 @@ def __init__(self,
self.enable_async = enable_async
self.is_async = self.enable_async and have_async_gen

self.native = native
if self.native:
self.code_generator_class = NativeCodeGenerator

This comment has been minimized.

@ThiefMaster

ThiefMaster May 1, 2017
Member

I don't like this. I'd rather add a native_code_generator_class class attribute and pick this one here. Otherwise there's no way of using a custom native code generator class without setting it after instantiating the environment (instead of setting it on the class level on a subclass)

This comment has been minimized.

@davidism

davidism May 1, 2017
Member

Or just make this a completely separate NativeEnvironment, rather than an option on the base environment.

This comment has been minimized.

@jctanner

jctanner May 1, 2017
Author Contributor

Does that mean you'd prefer a subclassed Environment?

This comment has been minimized.

@davidism

davidism May 1, 2017
Member

Yes, I think that would be more clear.

@ThiefMaster
Copy link
Member

@ThiefMaster ThiefMaster commented May 1, 2017

Not sure if this really its in the Jinja core. Any chance this would be possible fully on the Ansible side by using a custom Environment/CodeGenerator/Runtime (I made the last two overridable on the Environment level some time ago)

native_env = Environment(native=True)
native_tmpl = native_env.from_string("{% for x in listone %}{{ x }}{% endfor %}")
result = native_tmpl.render(listone=['a', 'b', 'c', 'd'])
assert isinstance(result, unicode)

This comment has been minimized.

@davidism

davidism May 1, 2017
Member

text_type not unicode

@davidism
Copy link
Member

@davidism davidism commented May 1, 2017

Possible concat implementation:

def concat(nodes):
    head = list(islice(nodes, 2))

    if not head:
        return None

    if len(head) == 1:
        out = head[0]
    else:
        out = u''.join([text_type(v) for v in nodes])

    try:
        return literal_eval(out)
    except (ValueError, SyntaxError, MemoryError):  # possibly RecursionError
        return out
@jctanner
Copy link
Contributor Author

@jctanner jctanner commented May 1, 2017

@davidism that concat example breaks this test ...

        native_env = Environment(native=True)
        native_tmpl = native_env.from_string("{% for x in listone %}{{ x }}{% endfor %}")
        result = native_tmpl.render(listone=['a', 'b', 'c', 'd'])
        assert isinstance(result, unicode)
        assert result == 'abcd'

The result is "cd", which I presume comes from having iterated over the first two values in the generator and not being able to seek backwards.

@davidism
Copy link
Member

@davidism davidism commented May 1, 2017

chain(head, nodes) fixes that.

@jctanner
Copy link
Contributor Author

@jctanner jctanner commented May 2, 2017

@davidism @ThiefMaster I think I've addressed everything suggested up till now. Any further thoughts?

@davidism
Copy link
Member

@davidism davidism commented May 2, 2017

Needs documentation and changelog. Can be moved to a separate module, like the sandboxed env.

I still think this needs a clearer name than just "native". TypedOutputEnvironment? NativeTypeEnv? PythonTypeEnv? Something else? Or not?

@jctanner
Copy link
Contributor Author

@jctanner jctanner commented May 2, 2017

@davidism Would you want just the new code in environment.py moved to a separate file, or should I also move the new code in utils.py as well?

@davidism
Copy link
Member

@davidism davidism commented May 2, 2017

All of it.

@jctanner
Copy link
Contributor Author

@jctanner jctanner commented May 2, 2017

@davidism all code is in a separate "nativetypes.py" file now. I honestly don't know what makes sense in terms of naming the classes/files. I chose "native" because it espoused how I think about it, but I realize that's not how everyone thinks. If you or the rest of the team want to decide and pick a name, I'll do the renames.

For the docs, how much do you want in the docstrings versus in the "docs" directory? Is there a make script for the "docs" dir ... I didn't see anything obvious. How does the dev team build and examine docs prior to push?

@davidism
Copy link
Member

@davidism davidism commented May 2, 2017

Install sphinx, cd to docs, make html, open _build/html/index.html. Don't worry if the theme doesn't look the same. See docs/sandbox.rst as a possible template for your docs.

@jctanner
Copy link
Contributor Author

@jctanner jctanner commented May 4, 2017

@davidism first pass on webdocs is done. I wasn't sure where to put it in the TOC, so I placed last in the list, beneath tips n tricks.

Here's a rendered example http://tannerjc.net/tmp/jinja/html/nativetypes.html

Native Python Types
===================

The Jinja2 :class:`NativeEnvironment` class can be used instead of :class:`Environment` to override jinja's default behavior of returning only strings. This can be useful if you are using jinja outside the context of creating text files.

This comment has been minimized.

@davidism

davidism May 9, 2017
Member

"Jinja", without 2, capitalized.



def native_concat(nodes):
'''

This comment has been minimized.

@davidism

davidism May 9, 2017
Member

Use double quotes, remove blank first line, for all docstrings.


class NativeCodeGenerator(CodeGenerator):
'''
A custom code generator, which avoids injecting to_string() calls around

This comment has been minimized.

@davidism

davidism May 9, 2017
Member

``to_string()``

def visit_Output(self, node, frame):
'''
Slightly modified from the same method in CodeGenerator,

This comment has been minimized.

@davidism

davidism May 9, 2017
Member

:class:CodeGenerator (linked appropriately)

def visit_Output(self, node, frame):
'''
Slightly modified from the same method in CodeGenerator,
so that to_string() is not inserted afer the yield keyword.

This comment has been minimized.

@davidism

davidism May 9, 2017
Member

``to_string()``

def test_loops(self, env):

# FIXME - is this what we want?

This comment has been minimized.

@davidism

davidism May 9, 2017
Member

Obsolete comment?


def test_loop_look_alike(self, env):

# FIXME - conflicts with test_loops

This comment has been minimized.

@davidism

davidism May 9, 2017
Member

Obsolete comment?

:copyright: (c) 2017 by the Jinja Team.
:license: BSD, see LICENSE for more details.
"""

This comment has been minimized.

@davidism

davidism May 9, 2017
Member

Remove header and encoding.

"""
import pytest

from jinja2 import Markup, Environment

This comment has been minimized.

@davidism

davidism May 9, 2017
Member

Clean up unused imports.



@pytest.mark.test_tests
class TestNativeTestsCase(object):

This comment has been minimized.

@davidism

davidism May 9, 2017
Member

TestNativeEnvironment

@@ -12,6 +12,7 @@ Jinja2 Documentation
integration
switching
tricks
nativetypes

This comment has been minimized.

@davidism

davidism May 9, 2017
Member

This makes more sense below sandbox.

@davidism
Copy link
Member

@davidism davidism commented May 9, 2017

I'm still hesitant to add this to Jinja. It's such a standalone thing that it would make sense as a Jinja-NativeEnv package.

Our philosophy is to avoid adding more features unless they're absolutely necessary, since we have limited resources and want to keep the packages focused on one obvious use case. "Render to Python types" is not an obvious use case, given the features in the rest of the library.

Will you be willing to monitor this repository and keep this feature in sync with the rest of the code? Is there a plan within Ansible to provide maintenance for this?

@untitaker @ThiefMaster can you help make a decision about this?

@sivel
Copy link
Contributor

@sivel sivel commented May 9, 2017

Speaking from my POV, and not on behalf of @jctanner

I had initially voiced concern about this living outside of jinja, as the likelihood for a change to crop up in jinja, that impacts this code could be pretty high.

My initial recommendation was to get buy in from the authors of jinja, to keep this in mind, and perform external validation to ensure they were not breaking the code in Ansible to perform this, and potentially keeping us informed of such changes. Obviously, this route has some problems.

Including in jinja helps us ensure that they code is functional. However at some level I do imagine we would need to vendor this code as well, to support older versions of jinja, such as those an OS packaging system would provide.

We already have some of this problem currently. An example was the 2.9 release, where changes broke some things for our users, that we had to deal with it, because we aren't tightly integrated from a community perspective.

I'm not necessarily recommending a particular solution, just voicing some concerns.

@jctanner
Copy link
Contributor Author

@jctanner jctanner commented May 9, 2017

Will you be willing to monitor this repository and keep this feature in sync with the rest of the code? Is there a plan within Ansible to provide maintenance for this?

We are going to have to "vendor" a copy of this code for older versions of jinja that our consumers might have, so yeah we have to keep ourselves in sync with upstream and do everything we can to keep the upstream functional.

My eventual goal is to see if the jinja dev team would consider allowing me to break up CodeGenerator.visitOutput into a few smaller functions. If so, the nativetypes code can be drastically smaller and maintenance will be much simpler. I did not want to muddy the waters and confuse that refactor with this feature, so I didn't bring it up yet.

Our philosophy is to avoid adding more features unless they're absolutely necessary, since we have limited resources and want to keep the packages focused on one obvious use case. "Render to Python types" is not an obvious use case, given the features in the rest of the library.

I fully understand that mentality. I'm still trying to build a list of useful examples of this feature that make sense outside ansible, but I also think that the community might have some input too.

@mitsuhiko
Copy link
Member

@mitsuhiko mitsuhiko commented May 30, 2017

I think I'm generally in favour of adding that. In the past I used similar things. I will have a look at this.

@mitsuhiko mitsuhiko self-assigned this May 30, 2017
finalize = lambda x: text_type(
self.environment.finalize(self.environment, x))
else:
finalize = lambda x: text_type(self.environment.finalize(x))

This comment has been minimized.

@davidism

davidism Jun 5, 2017
Member

Should this text_type be removed?

else:
finalize = lambda x: text_type(self.environment.finalize(x))
else:
finalize = text_type

This comment has been minimized.

@davidism

davidism Jun 5, 2017
Member

Should this be replaced with a no-op?

close = 0
if frame.eval_ctx.volatile:
self.write('(escape if context.eval_ctx.autoescape'
' else to_string)(')

This comment has been minimized.

@davidism

davidism Jun 5, 2017
Member

Should this be removed? Does autoescaping even make sense for native types?

close = 0
if frame.eval_ctx.volatile:
self.write('(escape if context.eval_ctx.autoescape else'
' to_string)(')

This comment has been minimized.

@davidism

davidism Jun 5, 2017
Member

Should this be removed? Does autoescaping even make sense for native types?

getattr(func, 'evalcontextfunction', False):
allow_constant_finalize = False
elif getattr(func, 'environmentfunction', False):
finalize = lambda x: text_type(

This comment has been minimized.

@davidism

davidism Jun 5, 2017
Member

Should this text_type be removed?

@jctanner
Copy link
Contributor Author

@jctanner jctanner commented Jun 8, 2017

@davidism I removed all of the text_type refs and set the finalize noop per your suggestions. Tests still seem to pass for me.

@jctanner
Copy link
Contributor Author

@jctanner jctanner commented Jun 20, 2017

anything else I can do to help out here?

@davidism
Copy link
Member

@davidism davidism commented Oct 31, 2017

It's been a while (sorry!) and there was some confusion over what we were waiting on here. https://botbot.me/freenode/pocoo/2017-10-31/?msg=92959650&page=3 Our conclusion was that adding has_safe_repr there was appropriate and fixed the issue I came up with.

@davidism davidism merged commit d17c7db into pallets:master Oct 31, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jctanner
Copy link
Contributor Author

@jctanner jctanner commented Oct 31, 2017

@davidism thank you so much! You've made many ansible devs happy today!

@davidism
Copy link
Member

@davidism davidism commented Nov 8, 2017

@jctanner I just released Jinja 2.10 with this included. I tried to ping you on Twitter but couldn't find your username. 😄

@jctanner
Copy link
Contributor Author

@jctanner jctanner commented Nov 9, 2017

@davidism I do all my microblogging in github comments =)

New downstream patch for ansible ansible/ansible#32738

@arodier
Copy link

@arodier arodier commented Apr 2, 2018

Hello,
Thank for your hard work. In which version of Ansible this has been fixed?

@jctanner
Copy link
Contributor Author

@jctanner jctanner commented Apr 2, 2018

@arodier we're going to merge ansible/ansible#32738 at the very beginning of the ansible 2.7 development cycle.

@arodier
Copy link

@arodier arodier commented Apr 2, 2018

Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.