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

Use Jinja2 to generate job scripts #338

Closed
wtbarnes opened this issue Sep 11, 2019 · 3 comments
Closed

Use Jinja2 to generate job scripts #338

wtbarnes opened this issue Sep 11, 2019 · 3 comments

Comments

@wtbarnes
Copy link
Member

Currently, each job script is generated by a combination of old-style (i.e. not f-strings) string interpolation and joining of lists of strings. Using a templating language like Jinja would simplify this approach and offload the string formatting to a separate file.

It looks like the possibility of using jinja2 was discussed in #321 and #133. Is there a consensus on whether this is a good idea? It would certainly clean up some of the code, but maybe at the cost of readability as it would offload a lot of the logic (e.g. whether to include certain flags/options) to a different file.

If there's interest in this approach, I'd be happy to take a crack at it.

@lesteve
Copy link
Member

lesteve commented Sep 11, 2019

I think the consensus is that there is definitely an interest in using Jinja2 and we would certainly welcome an attempt to do it.

I would advise waiting for #307 (refactoring to use SpecCluster) to be merged before having a go at it. I would expect this is a matter of days before we can merge this in master.

I am going to close this issue in favour of #133. If you can post a comment in #133 to say you are interested to work on this, it would be great.

Side-comment: we are still supporting Python 3.5 so we can not use f-strings.

@lesteve lesteve closed this as completed Sep 11, 2019
@wtbarnes
Copy link
Member Author

Thanks for the feedback @lesteve.

I would advise waiting for #307 (refactoring to use SpecCluster) to be merged before having a go at it. I would expect this is a matter of days before we can merge this in master.

For some reason I thought this had already been merged. I'll definitely wait on that. I'm not in a huge hurry!

I am going to close this issue in favour of #133. If you can post a comment in #133 to say you are interested to work on this, it would be great.

Will do

Side-comment: we are still supporting Python 3.5 so we can not use f-strings.

I figured as much.

@lesteve
Copy link
Member

lesteve commented Sep 25, 2019

I would advise waiting for #307 (refactoring to use SpecCluster) to be merged before having a go at it. I would expect this is a matter of days before we can merge this in master.

FYI #307 has been merged. If you want to consider working on this again, that'd be great.

If you are looking for smaller issues to work on, #332 could be a good one to get started. Other than this #122 would be a great one to get to the bottom of. In particular having a snippet to reproduce the problem would be a great start (see #122 (comment) for what I would try).

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

No branches or pull requests

2 participants