Add templates using jinja2 #107

joseph-sentry · 2023-09-21T14:50:27Z

This commit creates the TemplateService that uses
jinja to get and render templates.

This TemplateService provides the get_template method to get a rendered template. It takes the file name of the template as an arg and the template variables as the kwargs.

Templates should be added into the templates directory at the top level of the repository. This commit creates two example templates.

codecov · 2023-09-21T15:00:53Z

Codecov Report

Merging #107 (e570cc6) into main (a7fc0fd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #107   +/-   ##
=======================================
  Coverage   98.45%   98.45%           
=======================================
  Files         364      366    +2     
  Lines       26989    27023   +34     
=======================================
+ Hits        26571    26605   +34     
  Misses        418      418

Flag	Coverage Δ
integration	`98.42% <100.00%> (+<0.01%)`	⬆️
latest-uploader-overall	`98.42% <100.00%> (+<0.01%)`	⬆️
onlysomelabels	`98.45% <100.00%> (+<0.01%)`	⬆️
unit	`98.42% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
NonTestCode	`97.04% <100.00%> (+<0.01%)`	⬆️
OutsideTasks	`98.20% <100.00%> (+<0.01%)`	⬆️

Files	Coverage Δ
services/template.py	`100.00% <100.00%> (ø)`
services/tests/test_template.py	`100.00% <100.00%> (ø)`

This change has been scanned for critical changes. Learn more

codecov-staging · 2023-09-21T15:01:14Z

Codecov Report

Merging #107 (e570cc6) into main (a7fc0fd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #107   +/-   ##
=======================================
  Coverage   93.19%   93.20%           
=======================================
  Files         341      343    +2     
  Lines       26645    26679   +34     
=======================================
+ Hits        24832    24866   +34     
  Misses       1813     1813

Flag	Coverage Δ
integration	`93.20% <100.00%> (+<0.01%)`	⬆️
latest-uploader-overall	`93.20% <100.00%> (+<0.01%)`	⬆️
unit	`93.20% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
NonTestCode	`93.91% <100.00%> (+<0.01%)`	⬆️
OutsideTasks	`96.69% <100.00%> (+<0.01%)`	⬆️

Files	Coverage Δ
services/template.py	`100.00% <100.00%> (ø)`
services/tests/test_template.py	`100.00% <100.00%> (ø)`

codecov-public-qa · 2023-09-21T15:01:45Z

Codecov Report

Merging #107 (e570cc6) into main (a7fc0fd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #107   +/-   ##
=======================================
  Coverage   93.19%   93.20%           
=======================================
  Files         341      343    +2     
  Lines       26645    26679   +34     
=======================================
+ Hits        24832    24866   +34     
  Misses       1813     1813

Flag	Coverage Δ
integration	`93.20% <100.00%> (+<0.01%)`	⬆️
latest-uploader-overall	`93.20% <100.00%> (+<0.01%)`	⬆️
unit	`93.20% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
NonTestCode	`93.91% <100.00%> (+<0.01%)`	⬆️
OutsideTasks	`96.69% <100.00%> (+<0.01%)`	⬆️

Files Changed	Coverage Δ
services/template.py	`100.00% <100.00%> (ø)`
services/tests/test_template.py	`100.00% <100.00%> (ø)`

giovanni-guidini · 2023-09-21T18:18:01Z

services/template.py

+    return _get_cached_template_service()
+
+
+def _get_cached_template_service():


Do we need this to be a singleton?...
I ask because it doesn't seem to be a pattern we use much with services in the worker, and I don't know if it provides benefits to implement it like that.

My real concern is that we had issues before with tests (read ATS) around the cached ENV.

I'm not opposed to this idea, I'm just curious what are the benefits of doing it as a singleton. Does the loading process takes long? Is that recommended?

I don't think it has to be, I guess it does add complexity for no real reason, i just thought it's probably an object that we don't need to instantiate a bunch of times, it's sort of just like load it and have it stay the same until we redeploy, but you're correct that we don't need it to be a singleton

giovanni-guidini · 2023-09-28T12:21:48Z

services/template.py

+from jinja2 import Environment, PackageLoader, StrictUndefined, select_autoescape
+
+
+def get_template_service():


I think this is unnecessary. We can very well just instantiate TemplateService() directly

giovanni-guidini · 2023-09-28T12:24:11Z

services/template.py

+            undefined=StrictUndefined,
+        )
+
+    def get_template(self, name, **kwargs):


[nit] change "name" to "template_name".

[less nit]
Personally I don't love that we return a rendered template from this function. I think that this function should simply get and return the template we specified, AND handle errors when we fail to get the template (like you ask for one that doesn't exist)

Then we modify the template object as we see fit and render it ourselves later on.

So I do think the responsibilities of this function should be limited to fetching a template and returning it (as the name suggests) and handling errors.

giovanni-guidini · 2023-09-28T12:27:02Z

services/tests/test_template.py

+from services.template import get_template_service
+
+
+class TestTemplate(object):


I'd like to see a test where you ask for a template that doesn't exist

matt-codecov

all good to me, except unfortunately i have to give you whiplash on the singleton issue :v

i googled "jinja environment expensive" and learned you were on the right track making it a singleton before. i think gio's feedback about following the FooService().bar() convention of other services in our code is also valid, but we can bridge the gap under the hood

matt-codecov · 2023-09-28T23:05:47Z

services/template.py

+class TemplateService:
+    def __init__(self):
+        # this loads the templates from the templates directory in this repository since it's looking for a dir named `templates` next to app.py
+        self.env = Environment(


so, regarding using a singleton here:

https://jinja.palletsprojects.com/en/3.0.x/api/

Most applications will create one Environment object on application initialization and use that to load templates. In some cases however, it’s useful to have multiple environments side by side, if different configurations are in use.

https://stackoverflow.com/questions/618827/optimizing-jinja2-environment-creation

My application is running on Google App Engine and most of requests constantly gets yellow flag due to high CPU usage. Using profiler I tracked the issue down to the routine of creating jinja2.Environment instance.

it looks like creating an Environment is expensive so we should try to only do it once. either that or we precompile templates with something like https://github.com/MiCHiLU/jinja2-precompiler but that approach doesn't seem well-documented.

i don't have a strong opinion on how to accomplish this. you could make env a global or class-level variable instead of an instance variable and be done with it, or you could make TemplateService a real singleton again. if you go the singleton route, there's a way to make it a singleton that you still invoke like TemplateService().get_template(): https://code.activestate.com/recipes/66531/ (maybe not technically a singleton but splitting hairs)

@giovanni-guidini what were the "cached ENV" issues you alluded to? were those, like, env vars or specifically a jinja2.Environment?

matt-codecov

looks great, tiny tweak!

matt-codecov · 2023-09-29T17:32:20Z

services/template.py

+
+    def __init__(self):
+        # this loads the templates from the templates directory in this repository since it's looking for a dir named `templates` next to app.py
+        TemplateService.env = Environment(


this will reinitialize TemplateService.env each time we create an instance, we should only initialize it if it's still None

This commit creates the TemplateService that uses jinja to get and render templates. This TemplateService provides the get_template method to get a rendered template. It takes the file name of the template as an arg and the template variables as the kwargs. Templates should be added into the templates directory at the top level of the repository. This commit creates two example templates. Signed-off-by: joseph-sentry <[email protected]>

Signed-off-by: joseph-sentry <[email protected]>

joseph-sentry force-pushed the joseph/template-service branch from 8fd1534 to 838f924 Compare September 21, 2023 15:01

joseph-sentry force-pushed the joseph/template-service branch from 838f924 to 3c05125 Compare September 21, 2023 15:02

joseph-sentry marked this pull request as ready for review September 21, 2023 15:02

joseph-sentry requested a review from giovanni-guidini September 21, 2023 15:02

joseph-sentry mentioned this pull request Sep 21, 2023

Add send email task and testing for the task using mailhog #111

Merged

giovanni-guidini reviewed Sep 21, 2023

View reviewed changes

joseph-sentry force-pushed the joseph/template-service branch from 3c05125 to 73c8150 Compare September 27, 2023 14:29

joseph-sentry requested review from giovanni-guidini, scott-codecov, matt-codecov and dana-yaish September 27, 2023 14:30

giovanni-guidini reviewed Sep 28, 2023

View reviewed changes

joseph-sentry force-pushed the joseph/template-service branch from 73c8150 to 471e7f4 Compare September 28, 2023 17:53

matt-codecov requested changes Sep 28, 2023

View reviewed changes

matt-codecov approved these changes Sep 29, 2023

View reviewed changes

joseph-sentry added 7 commits September 29, 2023 16:17

Make template service not a singleton

b2cbda3

Signed-off-by: joseph-sentry <[email protected]>

address feedback

baca28c

Signed-off-by: joseph-sentry <[email protected]>

lint and fix tests

d7ad99c

Signed-off-by: joseph-sentry <[email protected]>

make a singleton again, but without get_template_service

9238ccb

Signed-off-by: joseph-sentry <[email protected]>

small fix to TemplateService

23f0a53

Signed-off-by: joseph-sentry <[email protected]>

Remove loaded method from template service

e570cc6

Signed-off-by: joseph-sentry <[email protected]>

joseph-sentry force-pushed the joseph/template-service branch from 24aca61 to e570cc6 Compare September 29, 2023 20:17

joseph-sentry merged commit 3428227 into main Sep 29, 2023
31 of 32 checks passed

joseph-sentry deleted the joseph/template-service branch September 29, 2023 20:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add templates using jinja2 #107

Add templates using jinja2 #107

joseph-sentry commented Sep 21, 2023

codecov bot commented Sep 21, 2023 •

edited

Loading

codecov-staging bot commented Sep 21, 2023 •

edited

Loading

codecov-public-qa bot commented Sep 21, 2023 •

edited

Loading

giovanni-guidini Sep 21, 2023

joseph-sentry Sep 21, 2023

giovanni-guidini Sep 28, 2023

giovanni-guidini Sep 28, 2023

giovanni-guidini Sep 28, 2023

matt-codecov left a comment

matt-codecov Sep 28, 2023

matt-codecov left a comment

matt-codecov Sep 29, 2023

		return _get_cached_template_service()


		def _get_cached_template_service():

		from jinja2 import Environment, PackageLoader, StrictUndefined, select_autoescape


		def get_template_service():

		from services.template import get_template_service


		class TestTemplate(object):

Add templates using jinja2 #107

Add templates using jinja2 #107

Conversation

joseph-sentry commented Sep 21, 2023

codecov bot commented Sep 21, 2023 • edited Loading

Codecov Report

codecov-staging bot commented Sep 21, 2023 • edited Loading

Codecov Report

codecov-public-qa bot commented Sep 21, 2023 • edited Loading

Codecov Report

giovanni-guidini Sep 21, 2023

Choose a reason for hiding this comment

joseph-sentry Sep 21, 2023

Choose a reason for hiding this comment

giovanni-guidini Sep 28, 2023

Choose a reason for hiding this comment

giovanni-guidini Sep 28, 2023

Choose a reason for hiding this comment

giovanni-guidini Sep 28, 2023

Choose a reason for hiding this comment

matt-codecov left a comment

Choose a reason for hiding this comment

matt-codecov Sep 28, 2023

Choose a reason for hiding this comment

matt-codecov left a comment

Choose a reason for hiding this comment

matt-codecov Sep 29, 2023

Choose a reason for hiding this comment

codecov bot commented Sep 21, 2023 •

edited

Loading

codecov-staging bot commented Sep 21, 2023 •

edited

Loading

codecov-public-qa bot commented Sep 21, 2023 •

edited

Loading