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

Add empty jinja block #458

Merged
merged 14 commits into from
Feb 26, 2021
Merged

Conversation

berceanu
Copy link
Contributor

@berceanu berceanu commented Feb 8, 2021

Address #457.

@berceanu berceanu requested review from a team as code owners February 8, 2021 19:20
@berceanu berceanu requested review from csadorf and tommy-waltmann and removed request for a team February 8, 2021 19:20
@codecov
Copy link

codecov bot commented Feb 8, 2021

Codecov Report

Merging #458 (91cdca8) into master (e684554) will decrease coverage by 0.29%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
- Coverage   71.63%   71.34%   -0.30%     
==========================================
  Files          30       30              
  Lines        2958     2949       -9     
  Branches      547      543       -4     
==========================================
- Hits         2119     2104      -15     
- Misses        696      702       +6     
  Partials      143      143              
Impacted Files Coverage Δ
flow/project.py 75.37% <0.00%> (-0.57%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e684554...5f55973. Read the comment docs.

@vyasr vyasr self-requested a review February 9, 2021 02:11
vyasr
vyasr previously requested changes Feb 9, 2021
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

@berceanu can you also modify stampede2.sh? You'll notice that it completely overwrites the body block, and the new body block has two separate branches based on a variable use_launcher. The branch corresponding to use_launcher == False should look basically like the body block from base_script.sh, and you can copy-paste this solution in there as well.

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

This might be the academic within me, but can we maybe name the block pre_operation?

flow/templates/stampede2.sh Outdated Show resolved Hide resolved
@berceanu berceanu requested a review from vyasr February 23, 2021 18:25
flow/templates/stampede2.sh Outdated Show resolved Hide resolved
Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

I'm going to attempt to test this PR (generating a sample script for Stampede2 and checking to see if it makes sense -- probably not going to run it).

flow/templates/stampede2.sh Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor

vyasr commented Feb 25, 2021

@bdice thanks for testing on Stampede2, that was going to be my main outstanding request. Once you are convinced that tests look ok (perhaps @b-butler can also take a look since he has looked at Stampede2 scripts before) @berceanu will need to regenerate the template testing scripts. See here for how to do that, but please hold off committing new versions of the testing output until @bdice confirms that it looks correct.

@bdice
Copy link
Member

bdice commented Feb 25, 2021

I tried a few different things here but did not succeed and don't have time to do any more at the moment. In the variations I tried, either the offsets were wrong or the template failed to render entirely. We might need to just revert the changes to the Stampede2 template so that it retains the previous behavior. I really don't like that machine, it presents so many problems for signac-flow...

@vyasr
Copy link
Contributor

vyasr commented Feb 26, 2021

@bdice I think I've resolved the problem, aside from the known issue of pretend outputs not incrementing the counter correctly (see here for a refresher on that). Can you test on stampede again when you get a chance, and see if you're still getting output that you don't think makes sense? There's no way to validate the offset itself without actually submitting, but if you run the template tests and you see that the offset is always 0 in both cases (what I'd expect) then the output seems consistent.

If that seems right to you, then the reference output needs to be regenerated and this PR should be good to go.

Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

@vyasr your changes appear to work for me. Thanks! I did not test on the actual Stampede2 machine but I verified that the generated templates from our tests all say export _FLOW_STAMPEDE_OFFSET_=0.

@bdice bdice dismissed vyasr’s stale review February 26, 2021 20:15

Changes resolved.

@bdice bdice enabled auto-merge (squash) February 26, 2021 20:16
@bdice
Copy link
Member

bdice commented Feb 26, 2021

Ugh, I committed with a custom setting in my $HOME/.signac.rc. Let me fix that.

@bdice bdice force-pushed the feature/base_script_block branch from 864cc78 to 5f55973 Compare February 26, 2021 20:18
@bdice bdice merged commit e8fe8d3 into glotzerlab:master Feb 26, 2021
@berceanu berceanu deleted the feature/base_script_block branch March 3, 2021 18:23
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.

5 participants