Skip to content

251 use jinja for job script rendering#254

Merged
bschroeter merged 6 commits intomainfrom
251-use-jinja-for-job-script-rendering
Feb 21, 2024
Merged

251 use jinja for job script rendering#254
bschroeter merged 6 commits intomainfrom
251-use-jinja-for-job-script-rendering

Conversation

@bschroeter
Copy link
Copy Markdown
Collaborator

@bschroeter bschroeter commented Feb 16, 2024

Replaced in-source job script generation with Jinja templating.

For context - this simplifies the generation of job scripts (and indeed anything that interpolates into a template), meaning that we can remove the multiline string blocks, which are sensitive to whitespace, and abstract them into separate files.

This results in cleaner and more flexible code going forward and opens up opportunities to change the underlying templates at a moments notice.

@bschroeter bschroeter linked an issue Feb 16, 2024 that may be closed by this pull request
Comment thread ruff.toml Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b65ddbb) 60.76% compared to head (623c0a3) 61.15%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #254      +/-   ##
==========================================
+ Coverage   60.76%   61.15%   +0.38%     
==========================================
  Files          30       30              
  Lines        2238     2260      +22     
==========================================
+ Hits         1360     1382      +22     
  Misses        878      878              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

#PBS -l storage={{storage}}

module purge
{% for module in modules -%}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the -%, which controls whitespace generation in Jinja.

https://jinja.palletsprojects.com/en/latest/templates/#whitespace-control

@bschroeter
Copy link
Copy Markdown
Collaborator Author

All tests pass btw.

PBS Job Id: 108301484.gadi-pbs
Job Name:   benchmark_cable_qsub.sh
Execution terminated
Exit_status=0
resources_used.cpupercent=371
resources_used.cput=00:05:21
resources_used.jobfs=0b
resources_used.mem=601672kb
resources_used.ncpus=18
resources_used.vmem=601672kb
resources_used.walltime=00:01:39

Copy link
Copy Markdown
Member

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small comment about typing. Otherwise all good. I approve but it would be good to fix the typing before merging if I am correct in my comment.

Comment on lines +95 to +96
template = load_package_data(template_file)
return interpolate_string_template(template, **kwargs)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above it says load_package_data() outputs a dictionary but interpolate_string_template() takes a string as input.

I'm guessing the typing of the function's return value for load_package_data() needs to be updated to be Union[str, dict]?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes you are correct, now that we are potentially reading raw data this is needed. I'll update that now.

@bschroeter bschroeter merged commit 3783439 into main Feb 21, 2024
@bschroeter bschroeter deleted the 251-use-jinja-for-job-script-rendering branch February 21, 2024 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use Jinja for job script rendering.

2 participants