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

[CT-258] [Spike] Upgrade to Jinja3? #4748

Closed
jtcohen6 opened this issue Feb 18, 2022 · 11 comments · Fixed by #5465
Closed

[CT-258] [Spike] Upgrade to Jinja3? #4748

jtcohen6 opened this issue Feb 18, 2022 · 11 comments · Fixed by #5465
Assignees
Labels
dependencies Changes to the version of dbt dependencies spike tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality
Milestone

Comments

@jtcohen6
Copy link
Contributor

Picking up from #4745 (comment):

We've hesitated to do this, given the risk of breaking changes to user/project code—but we need to weigh that against the risk of running into this type of dependency/installation issue, given that Jinja2 is no longer officially supported and won't be patched accordingly.

Let's take the time to look through the set of (breaking) changes, and predict the impact on user/project code. Based on that analysis, we should move ahead with an upgrade, or consider our alternatives. Forking/vendoring Jinja2 is far from ideal, but it is an option on the table.

@jtcohen6 jtcohen6 added dependencies Changes to the version of dbt dependencies tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality labels Feb 18, 2022
@jtcohen6 jtcohen6 added this to the v1.1 milestone Feb 18, 2022
@github-actions github-actions bot changed the title Upgrade to Jinja3? [CT-258] Upgrade to Jinja3? Feb 18, 2022
@leahwicz leahwicz changed the title [CT-258] Upgrade to Jinja3? [CT-258] [Spike] Upgrade to Jinja3? Feb 28, 2022
@leahwicz
Copy link
Contributor

@jtcohen6 did you run this with Jinja3?

@leahwicz
Copy link
Contributor

Goal: What are our most viable options for leaving Jinja2? If so what is the impact or work that needs to be done? If moving moving to Jinja3, what is the plan?

Potential Options:

  1. Upgrade to Jinja3
  2. Fork Jinja2
  3. Own Jinja parser
  4. Anything else you can think of

@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Mar 2, 2022

@jtcohen6 did you run this with Jinja3?

  • Our integration tests seem to be passing with Jinja2==3.0.3, it's just mypy that's failing: Bump jinja2 from 2.11.3 to 3.0.3 in /core #4262
  • I've managed to successfully parse + run our internal analytics project with Jinja2==3.0.3 installed

@jtcohen6 jtcohen6 added the spike label Mar 2, 2022
@nathaniel-may
Copy link
Contributor

If it's simple to upgrade to jinja 3 we will likely do that. so that will be the first step of the spike. Outcome of this spike is a write up. write up includes consequences if we stay with jinja 2.

@jaklan
Copy link

jaklan commented Jun 16, 2022

Hi guys, when can we expect the release supporting Jinja3? At the moment - I'm not able to create an environment for generating both mkdocs and dbt docs (we deploy them together), because there's a version conflict between dbt-core and mkdocs-material related to jinja2:

  Because no versions of dbt-redshift match >1.1.0,<2.0.0
   and dbt-redshift (1.1.0) depends on dbt-postgres (>=1.1.0,<1.2.0), dbt-redshift (>=1.1.0,<2.0.0) requires dbt-postgres (>=1.1.0,<1.2.0).
  And because no versions of dbt-postgres match >1.1.1,<1.2.0
   and dbt-postgres (1.1.1) depends on dbt-core (1.1.1), dbt-redshift (>=1.1.0,<2.0.0) requires dbt-core (1.1.1) or dbt-postgres (>=1.1.0,<1.1.1).
  And because dbt-core (1.1.1) depends on Jinja2 (2.11.3)
   and mkdocs-material (8.3.6) depends on jinja2 (>=3.0.2), if dbt-redshift (>=1.1.0,<2.0.0) and mkdocs-material (8.3.6) then dbt-postgres (>=1.1.0,<1.1.1).
  And because no versions of mkdocs-material match >8.3.6,<9.0.0
   and temp depends on mkdocs-material (^8.3.6), dbt-redshift (>=1.1.0,<2.0.0) requires dbt-postgres (>=1.1.0,<1.1.1).
  So, because temp depends on both dbt-postgres (^1.1.1) and dbt-redshift (^1.1.0), version solving failed.

As a workaround I have to create 2 separate venvs.

@brunocastroibarburu94
Copy link

Same here

@jtcohen6 jtcohen6 modified the milestones: v1.2, v1.3 Jun 28, 2022
@emmyoop emmyoop self-assigned this Jul 11, 2022
@emmyoop emmyoop mentioned this issue Jul 12, 2022
6 tasks
@r-richmond
Copy link

DBT can no longer be installed in the same environment as Airflow after Airflow's most recent release 2.3.3 on 2022-07-05 due to a version conflict on Jinja.

Details on Jinja Conflict in latest Airflow Airflow updated their Flask App Builder to V4 which included a requirement of Jinja2 >= 3.0

Full Details

Questions

  1. After my naive glance at jinja2 v3 upgrade #5465 makes it seem like everything internal was compatible. Given this is it possible to include this version upgrade in 1.2?
  2. If 1 is untenable do you have thoughts on removing the specific version lock on Jinja2 to allow end users more flexibility in upgrading? i.e.
  • from "Jinja2==2.11.3",
  • to "Jinja2>=2.11.3",
  1. Thoughts on doing 2 as a temporary measure until you are comfortable bumping Jinja2 to 3.x forcefully.

@jtcohen6
Copy link
Contributor Author

We're going to attempt a Jinja3 upgrade as part of v1.3. It's a positive sign that our internal testing is all passing with v3.1.2, but we'll want extensive end user testing to check for subtle breaking changes. Our internal tests do not cover the full range of Jinja2 capabilities, on which users might be depending in their own code.

I'm hesitant to loosen dbt-core's version pin on Jinja, given how essential it is to so much user-space code, and the problems that we've encountered in the past during new minor + patch releases of Jinja. I appreciate that this does make it more difficult to install dbt-core in the same Python environment as other tools that depend on Jinja, such as Flask / Airflow. If we do bump to Jinja3 for v1.3, we should look to keep bumping the Jinja3 version in all new minor versions of dbt-core. After a period of time, during which we've successfully bumped the Jinja3 version without any other changes, and we observe zero regressions for end-user code, we can loosen the version pin.

@r-richmond
Copy link

r-richmond commented Jul 17, 2022

It's a positive sign that our internal testing is all passing with v3.1.2, but we'll want extensive end user testing to check for subtle breaking changes. Our internal tests do not cover the full range of Jinja2 capabilities, on which users might be depending in their own code.

Given dbt-core's cross compatibility & the concern around end-user code why keep/enforce the single version pin? By keeping the version pin in place it places all the responsibility/blame of jinja2 version problems on the maintainers of dbt-core instead of end-users environment maintainers. If dbt-core were to loosen the restriction dbt-core could stop worrying about it to a larger degree.

If we do bump to Jinja3 for v1.3, we should look to keep bumping the Jinja3 version in all new minor versions of dbt-core. After a period of time, during which we've successfully bumped the Jinja3 version without any other changes, and we observe zero regressions for end-user code, we can loosen the version pin.

If I'm reading this correctly, the order of actions here prevents end users from easily testing out Jinja2 V3. If the version pin was loosened before the jinja2 v3 update users could easily install the v3 version into their environment without fighting with version conflicts in pip/pip-like tools via additional version specs in their requirements.txt/pyprojet.toml/Pipfile/etc..

However, if you leave single version pin in place until after updating to Jinja2 V3.x. you prevent easy testing of the new Jinja2 version by end users. I.e. I'd like to test Jinja2 3.x with our project without any other dbt-core updates and I think that will require forking the project and modifying the pin instead of a simple version pin in our environment...

If you do leave the version in place how would you recommend end-users test the new v3.x of Jinja2 in their dbt-core projects?

@jtcohen6
Copy link
Contributor Author

@r-richmond It's a really fair point, and a balance we need to make. As I see it, there are two options:

  1. We keep relatively tight pins on any "critical" dependencies (Jinja, agate, mashumaro, networkx), where even subtle changes can have unintended or breaking consequences for end users. As dbt-core maintainers, we manage dependency upgrades within the larger process of preparing new dbt-core minor versions. Users try out new dependency versions as part of trying out a new minor version; there's a clear channel for feedback, and a clear next step (downgrade to previous minor version) if something goes awry.
  2. We loosen dependency pins, reserving them only for package versions which we know to be incompatible. (This was the case with Jinja2==3.0.0 right after its initial release.) On the plus side, we give end users more control over managing their own dependency upgrades, by specifying their own pins and testing out new versions; on the minus side, we require end users to manage their own dependency upgrades. It's totally possible that a new minor (or even patch!) version of Jinja is released, it makes backward-incompatible changes for dbt-core or for the code in someone's project, and they only find out the next time they pip install dbt-core && dbt run.

To date, we have preferred option 1 to option 2.

Future

That being said, we are reviewing our policies around dependencies, and my aim in this work is to arrive at a more-balanced approach. (There's an initial start in #5450, with more detailed policies in early draft state.) A balanced approach has two components:

  • Looser dependency pins in setup.py
  • Exact (==) dependency versions in a requirements.txt, which we compile + test + distribute with every dbt-core release, and for which we can guarantee total working functionality

Then, we can provide users with two options:

  • pip install -r requirements.txt, to start using dbt in a new Python environment, in a way that's guaranteed to work
  • pip install dbt-core, if you need to manage dbt-core within a more-complex Python environment, alongside other dependencies

@r-richmond
Copy link

r-richmond commented Jul 18, 2022

Then, we can provide users with two options:

  • pip install -r requirements.txt, to start using dbt in a new Python environment, in a way that's guaranteed to work
  • pip install dbt-core, if you need to manage dbt-core within a more-complex Python environment, alongside other dependencies

FWIW This would address all my concerns & is also what Airflow does (Loose Pip Constraints & Specific Officially Supported Constraints). So +1 to #5450 & here's to hoping that work can be completed soon so end-users can test/upgrade to Jinja2 3.x without forking. Thank you for your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Changes to the version of dbt dependencies spike tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants