refactor: Jinja experiment for AffiliateLinks template#12776
Conversation
![accesslint[bot]](https://avatars.githubusercontent.com/in/1502?s=60&v=4){kind=small}
github-project-automation
Bot
moved this to Waiting Review/Merge from Staff
in Ray's Project
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
This PR advances the Phase 3 work for #12678 by introducing a Jinja2 proof-of-concept port of the AffiliateLinks macro and adding an experiment path in AffiliateLinksPartial.generate_async() to render both Templetor and Jinja2, compare outputs, and log timing/match results when enabled via env var.
Changes:
- Add a new Jinja2 template port:
openlibrary/macros/AffiliateLinks.html.jinja. - Add a canary experiment path in
openlibrary/plugins/openlibrary/partials.pyto compare Templetor vs Jinja2 output and log match/timings behindOL_JINJA_EXPERIMENT=1. - Add a new test module covering Jinja rendering and (optionally) Templetor equivalence, and introduce Jinja2 as a dependency.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| requirements.txt | Adds Jinja2 dependency for the experiment/template rendering. |
| openlibrary/macros/AffiliateLinks.html.jinja | Jinja2 port of the existing Templetor AffiliateLinks macro. |
| openlibrary/plugins/openlibrary/partials.py | Adds Jinja2 rendering helpers + experiment gating/logging in AffiliateLinksPartial.generate_async(). |
| openlibrary/plugins/openlibrary/tests/test_jinja_experiment.py | New tests for Jinja rendering and output equivalence (with optional web.py-based comparison). |
| _JINJA_ENV = Environment( | ||
| loader=FileSystemLoader(Path(__file__).resolve().parent.parent.parent / "macros"), | ||
| autoescape=True, | ||
| ) | ||
| _JINJA_ENV.globals["_"] = _ | ||
|
|
||
| logger = logging.getLogger("openlibrary.partials") | ||
| _EXPERIMENT_LOGGER = logging.getLogger("openlibrary.jinja_experiment") | ||
| _JINJA_EXPERIMENT_ENABLED = os.getenv("OL_JINJA_EXPERIMENT") == "1" |
| t0 = _time.perf_counter() | ||
| macro = web.template.Template.globals["macros"].AffiliateLinks(primary_stores, more_stores) | ||
| return {"partials": str(macro)} | ||
| templetor_html = str(macro) | ||
| t1 = _time.perf_counter() | ||
|
|
for more information, see https://pre-commit.ci
RayBB
left a comment
RayBB
left a comment
There was a problem hiding this comment.
Great start! Can you make it so instead of rendering both for each request we accept a query parameter jinja=true and we can use that to switch between the two versions.
Also would be great if you checkout the jinja docs to ensure we follow best practices for setup here.
github-project-automation
Bot
moved this from Waiting Review/Merge from Staff
to Someone else is working on it
in Ray's Project
|
Hey @RayBB, I have switched to j |
github-actions
Bot
added
the
Needs: Response
| @@ -0,0 +1,33 @@ | |||
| {% macro affiliate_link(store) %} | |||
| <li class="prices-{{ store.key }}"> | |||
There was a problem hiding this comment.
WCAG 1.3.1: <li> is not contained in a <ul>, <ol>, or <menu>.
<li> elements must be contained in a <ul>, <ol>, or <menu>.
Details
List items (<li>) only have semantic meaning inside a list container (<ul>, <ol>, or <menu>). Outside of these containers, assistive technologies cannot convey the list relationship. Wrap <li> elements in the appropriate list container.
| {% endmacro %} | ||
|
|
||
| <span class="affiliate-links-section"> | ||
| <ul class="buy-options-table"> |
There was a problem hiding this comment.
WCAG 1.3.1: <ul> contains direct text content. Wrap in <li>.
<ul> and <ol> must only contain <li>, <script>, or <template> as direct children.
Details
Screen readers announce list structure ('list with 5 items') based on proper markup. Placing non-<li> elements directly inside <ul> or <ol> breaks this structure. Wrap content in <li> elements, or if you need wrapper divs for styling, restructure your CSS to style the <li> elements directly.
RayBB
added
On Testing
and removed
Needs: Response
RayBB
left a comment
RayBB
left a comment
There was a problem hiding this comment.
This is excellent work and gets us to a really solid place for experimenting with Jinja as a replacement for Templetor.
The implementation worked well enough in my testing that I went ahead and removed the feature flag entirely. Instead of having both paths, this endpoint will now just be served through Jinja.
For this particular page, I wasn't able to observe a meaningful performance difference between the Jinja and Templetor versions. The HTML being rendered is very simple, and the dominant cost appears to be Memcached lookups rather than template rendering itself. That said, performance was never the primary motivation for this work.
The biggest benefits are around developer experience. Jinja gives us much better editor support, including syntax highlighting, and uses a syntax that is more familiar and less finicky than Templetor. It should also be significantly easier for AI tools to work with, which is becoming increasingly important.
Another valuable part of this PR is the supporting tooling. The pre-commit hooks for formatting and linting templates, along with Python tests that verify templates compile successfully, give us a much stronger foundation for maintaining template quality going forward.
Overall, I think this PR puts us in a good position to start adopting Jinja when it makes sense. I don't think we're at a point where we should immediately begin a broad migration, but as we move HTML pages to FastAPI, there will be cases where pages need substantial restructuring anyway to separate out Python logic. In those situations, adopting Jinja at the same time seems like a reasonable choice.
If I were to suggest a next step, it would probably be exploring whether some of the shared page layout and common template structure could be moved into Jinja. However, that's a fairly large and potentially complicated effort, and I don't think it should take priority over other FastAPI-related work right now.
Thanks again for pushing this forward. This feels like an important piece of groundwork that gives us flexibility for future migrations without forcing us into any immediate large-scale changes.
github-project-automation
Bot
moved this from Someone else is working on it
to Done
in Ray's Project
Closes #12678
This is the phase 3 of the #12678. It converts AffiliateLinks.html.jinja to Jinja as a proof-of-concept for Jinja in the codebase.
Technical
AffiliateLinks.html.jinjaas a direct Jinja2 port of the TempletorAffiliateLinks.htmlmacroAffiliateLinksPartial.generate_async()that renders both Templetor and Jinja2 versions, compares HTML output, and logs match status + render timesOL_JINJA_EXPERIMENT=1env var, zero overhead when disabled (returns Templetor HTML)Jinja2>=3.1.6torequirements.txtTesting
pytest openlibrary/plugins/openlibrary/tests/test_jinja_experiment.py -vOL_JINJA_EXPERIMENT=1tofast_webservice incompose.override.yamlcurl "http://localhost:18080/partials/AffiliateLinks.json?data=%7B%22args%22%3A%5B%22Robinson+Crusoe%22%2C%7B%22isbn%22%3Anull%2C%22asin%22%3A%22cu31924011498676%22%2C%22prices%22%3Atrue%7D%5D%7D"docker compose logs fast_web | grep "Jinja2 experiment"match=True templetor_ms=X.XX jinja2_ms=X.XXhttp://localhost:8080/books/OL24152177Mand confirm "Buy" section renders correctlyScreenshot
N/A — backend experiment only
Stakeholders
@RayBB