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

feat: pass globals as context to jinja2 template #1532

Closed
shaunc opened this issue May 15, 2022 · 4 comments
Closed

feat: pass globals as context to jinja2 template #1532

shaunc opened this issue May 15, 2022 · 4 comments
Assignees
Labels
Community Issue/PR opened by the open-source community Issue: Feature Request New feature or improvement to existing feature

Comments

@shaunc
Copy link

shaunc commented May 15, 2022

Description

When configuring with jinja2 templates, I don't want variables embedded in templates as a source of authority.

Therefore, I have threaded through globals dict as context for configuration templates.

Context

Make jinja2 templates for configuration more maintainable.

Possible Implementation

#1542

@shaunc shaunc added the Issue: Feature Request New feature or improvement to existing feature label May 15, 2022
@merelcht merelcht added the Community Issue/PR opened by the open-source community label May 16, 2022
@datajoely
Copy link
Contributor

Hi @shaunc your problem and solution are very valid, but we're not in a place to accept this just yet. A couple of weeks I pitched a bit of a duct-tape fix that would open this up, but we as a maintainer group came to the conclusion that it was too risky for us to implement. The discussion on the PR is also relevant.

The main risks stem from a couple of major points:

  1. Configuration as a whole needs a revamp and we're still working our way through the design for this. Check out Synthesis of user research when using configuration in Kedro #891 to see our research on the topic. Since our next steps aren't locked in, we're a bit hesitant to introduce something here we can't easily remove going forward / without a breaking change.
  2. Since the config loader implementation has existed from day 1 we actually have some awkward debt and tight coupling we need to work through - your PR touches common.py when really all of that should be in the AbstractConfigLoader (or similar) interface.
  3. As a team and across our userbase there are varying schools of thought on how powerful / encouraged Jinja should be within Kedro. I'm a fan of this Google SRE article which highlights some of the pain points here and the awkward tension between whether config should be imperative or declarative.
  4. As @sheldontsen-qb raised on the PR above, there is an argument that we should decouple Jinja templating from our bespoke Templating approach (which there are arguments we should deprecate).

I appreciate my answer is also a little frustrating, to solve your problem today the best alternative I can suggest is defining a custom Jinja2ConfigLoader that works for your purposes. This is a high priority for us, but since it's so important we're trying to move methodically and purposefully.

@datajoely datajoely self-assigned this May 16, 2022
@shaunc
Copy link
Author

shaunc commented May 16, 2022

You could slip it in and maybe no one would notice? :)

[ok .. adding thoughts to bottom of the design discussion.]

#891 (comment)

@datajoely
Copy link
Contributor

Response for reference: #891 (comment)

@merelcht
Copy link
Member

Hi @shaunc, thanks for raising this issue and sharing your thoughts in the user research synthesis issue as well!

Echoing @datajoely here: this problem is very valid and improving configuration in Kedro is a high priority for the team. However, as you probably figured from the long conversation on the research piece, it's far from trivial and we want to make sure if we do an overhaul of how configuration works, we do it well and serve our users' needs properly. Improving the ways of working with Jinja2 is certainly on our radar. But as things stand now adding this support to the TemplatedConfligLoader might not be the best way of doing this. Personally, I'm starting to lean towards a separate Jinja2ConfigLoader that would offer a more robust way of working with and extending the Jinja2 functionality in Kedro.

For the time being, I'd suggest you do what Joel said and create a custom config loader that suits your use case best. If it's alright with you, I'll close your related PR for now, and we can continue the discuss this issue here.

We really appreciate your feedback and ideas on how to improve Kedro!

@merelcht merelcht added Help Wanted 🙏 Contribution task, outside help would be appreciated! and removed Help Wanted 🙏 Contribution task, outside help would be appreciated! labels Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Issue/PR opened by the open-source community Issue: Feature Request New feature or improvement to existing feature
Projects
None yet
Development

No branches or pull requests

4 participants