-
Notifications
You must be signed in to change notification settings - Fork 109
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
Enable export of environment variables plus lobster run as a command #326
Conversation
@janosh would be great if you could have a look at it. Ideally, we have this in the next custodian and atomate2 version as it would simplify running automated bonding analysis runs quite a lot. I changed 2 lines of code here. Any suggestions welcome. Alternative to this solution: changing both atomate and atomate2 code base to not convert the command into a list. I am a bit hesitant to do that as atomate is not very frequently updated. |
custodian/lobster/jobs.py
Outdated
@@ -87,7 +87,8 @@ def run(self, directory="./"): | |||
zopen(os.path.join(directory, self.stderr_file), "w", buffering=1) as f_err, | |||
): | |||
# use line buffering for stderr | |||
return subprocess.Popen(cmd, cwd=directory, stdout=f_std, stderr=f_err) # pylint: disable=R1732 | |||
cmd = " ".join(cmd) # to join split commands (e.g., from atomate and atomate2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmd = " ".join(cmd) # to join split commands (e.g., from atomate and atomate2) | |
if isinstance(cmd, list): | |
cmd = " ".join(cmd) # to join split commands (e.g., from atomate and atomate2) |
We could also do this to make it safer in case a string is transferred or replace it with one of the shlex commands:
cmd = " ".join(cmd) # to join split commands (e.g., from atomate and atomate2) | |
import shlex | |
if isinstance(cmd, list): | |
cmd = shlex.join(cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janosh maybe Sequence is better than list but I am not sure if we ever pass anything other than a list
custodian/lobster/jobs.py
Outdated
@@ -78,16 +78,16 @@ def setup(self, directory="./"): | |||
|
|||
def run(self, directory="./"): | |||
"""Runs the job.""" | |||
cmd = self.lobster_cmd | |||
cmd = " ".join(self.lobster_cmd) # join split commands (e.g., from atomate and atomate2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to support both str
and Sequence
for lobster_cmd
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it would be good. Just made a comment on that with two alternatives. You were faster :D
Thanks, @janosh ! |
Would be great if a new version could be released when the next pymatgen version is released as well. |
Summary
This will enable using "NUM_OMP_THREADS=48 /bin/lobster-command" as a command. With this, running VASP and LOBSTER within one job-script with jobflow and atomate2 only (without installing fireworks or jobflow remote) will be possible. If users rely on one 1 CPU node only, this should ensure good run times.
(cc @naik-aakash )