Enable some Jinja extensions and add datetime capabilities

[Rocketknight1]

This PR enables a few built-in Jinja features, specifically the loopcontrols extension. This allows a few new features in templates, most notably the ability to break or continue in loops, which was previously impossible.

Also, this PR adds the ability to get the current date/time with a call to strftime(), since this was requested by the Llama team. A new test_jinja_extensions test is added to check that these features work as intended.

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[amyeroberts]

Thanks for adding!

This PR is mixing two different things which should be tested separately. Once that's done I think it'll be good to go

[amyeroberts]

It's strftime but only for the now datetime object. It would be better to do something like def strftime_now(format)

[Rocketknight1]

Done!

[amyeroberts]

This test name is far too generic! Is it testing extensions can be used? testing specific extensions? which extensions?

[Rocketknight1]

Renamed to test_jinja_extensions_are_enabled

[Rocketknight1]

Actually, even better, split it into two tests with clearer names!

[amyeroberts]

Thanks for updating!

In general, I don't think adding strftime_now is a good idea - it's a highly specific functionality which isn't very flexible. We should push against adding lots of little disconnected features.

Now the tests have been decoupled - this PR should really be too (the extensions and datetime are unrelated)

[amyeroberts]

We shouldn't add functionalities like this into core tokenization code -- enabling certain things in a jinja template has nothing to do with tokenization and we risk bloating the module with many unrelated tools. Instead, I'd propose two things:

• We move strftime_now to its own module we import from e.g. jinja_template_utils or jinja_template_tools or [insert better name]. This would be a library where we can define all these different utilities. raise_exception should go there too.
• We make the jinja template more extensible, such that we don't have to hard code this for everyone. It's been requested - but why expose this utility for all template users? It won't be a sustainable way to grow and manage this. Instead we could have e.g. a jinja_globals mapping which is used to add each named function e.g.

jinja_globals = {
    "raise_exception": raise_exception,
    "strftime_now": strftime_now
}

and then have model-specific defaults for their own tokenizers / templates, if we want.

The second suggestion I feel less strong about - very open to other suggestions on how to avoid adding all this logic inside the tokenizers

[Rocketknight1]

Hmmn, moving it out of the tokenizer I agree with! However, I think I disagree with the model-specific jinja_globals mapping. The reason is that we'll have to hard-code everything users need as a Python function somewhere. We won't actually be able to let users add new functions with their model repo, because these extensions are arbitrary Python code and would require trust_remote_code=True.

Therefore, since we have to hardcode everything in the transformers codebase anyway, requiring specific extensions to be enabled in a model-specific jinja_globals adds confusion and extra model maintenance burden for no real gain - the only difference when an extension is enabled is that certain functions or keywords become usable inside Jinja.

Therefore, I think we should just enable all of the extensions we support all the time. I don't think there is any performance penalty, so the only reason to disable an extension would be if users want to use a variable name that clashes with an extension, and we want to discourage that anyway.

This might change in future if we have a "dangerous" extension that poses a security risk, and so we want to limit it only to models that absolutely require it, to avoid a malicious template being able to call it. I have no plans to do anything like that right now, though, so I think that feature can wait until it's needed, if it ever is.

[amyeroberts]

Nice - far clearer and cleaner tests!

[Rocketknight1]

@amyeroberts good advice on the code - I moved it all out of the base tokenizer and into chat_template_utils, which cleans up the base tokenizer a lot. I think keeping strftime_now is a good idea, though, for three reasons:

1. It fulfills a need that will be very common (templates want to get the current date/time as a string for their system message)
2. stftime() is flexible enough for a lot of formats
3. We can't really expose more complex classes/libraries/datetime objects in Jinja to cater for all possible needs, only callable functions like this. As single functions go, this is a pretty useful/general one!

[amyeroberts]

Thanks for iterating on this!

I disagree on how we should manage the extensions, but there aren't too many at the moment.

It will become increasingly difficult to manage if more of these get added. Different teams and model owners will have different requirements: we either need to maintain an extremely clean set of atomic functionalities which can be composed or risk a mess of overlapping functionalities with no clear way for users to know the best approach for performing certain tasks with the templates.

[Rocketknight1]

@amyeroberts Yeah, I understand! The problem is that the templates have to run in Python-jinja2, huggingface/jinja for JS, and Minijinja in Rust/TGI. This means I can't, for example, have composable functions that return datetime objects from the Python library, because those objects won't exist in the other languages!

That means I really do have to stick to functions that operate over simple types, and build for common use cases rather than universality. I think strftime_now is a decent compromise there. Also, we're not necessarily locked-in - if it turns out in future that we made a mistake, we can deprecate the function and search the Hub for templates using it and make automatic PRs to do it a different way.

[amyeroberts]

This means I can't, for example, have composable functions that return datetime objects from the Python library, because those objects won't exist in the other languages!

That's kind of my point! As we can't build a clean library of functions we should try to avoid adding functionality globally as it can't be well abstracted.

we can deprecate the function and search the Hub for templates using it and make automatic PRs to do it a different way.

Deprecation is hard and best avoided whenever possible. Searching on the hub is also no sufficient for catching everything -- people will have code running locally which we cannot automatically update. We've experiencing the pain of this in another situation with deprecation of some params in image processor configs, which we can never fully complete, so I'd rather we avoid this whenever possible

[Rocketknight1]

This is fair - I'll try to keep it to a minimum! Still, something like strftime_now() is necessary for a lot of templates, just because "Current Date: 23 Aug 2024" or something like that is an increasingly common component of standard system messages.