-
Notifications
You must be signed in to change notification settings - Fork 0
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
Batch timeseries analysis in slurm #120
Conversation
working here
…nto dev_parrallel_ts
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.
hey bud, nice and fast turnaround - I am a bit concerned about the actual submission process (call to subprocess
, see me comment) - I'd also think you should add job requirements, no? Also, I'd rename it to batch so and so, since it's not quite parallel-parallel 😁
bgcval2/parrellel_timeseries.py
Outdated
else: | ||
# Submit job: | ||
print('Submitting:', ' '.join(command_txt)) | ||
command1 = subprocess.Popen(command_txt) |
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.
I'd defo encase this in a try/except with except fishing for some key elements in stderr, or just printing the whole stderr to screen; if you don't pipe stderr as out it'll be hidden, and the user won't know why their jobs have not been submitted when they thought they have
oh and also - this things really do deserve a test, not thru and thru with SLURM submission, but everything up to that. I can write the test when it's about ready 👍 |
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.
let's make sure the piping is done correctly; also, have you tried this in practice? We don't need any special env
to pass to sbatch
do we? Like any special environment variables
Co-authored-by: Valeriu Predoi <[email protected]>
I've been using this for a few days and it works on jasmin. Your amendment to the subprocess also works too. If batch_timeseries fails you get normal python errors. If it fails inside inside the sbatch script, then you get error messages in the places that we tell it to fail. |
yeh that's how we want it to behave, so stdout can be piped to eg a file. Looks good, bud! Let me write a test for it! |
I'm not ready to merge. Still need to add documentation & maybe get @DrYool to try it. |
The next question I have:
|
…gcval2 into dev_parrallel_ts
Basically, the process for adding a new job,
I suspect that these can be merged into fewer commands! |
@ledm I added some test gubbins, make sure to pull or merge so there are no conflictseses |
The logo in the README points towards a file on the main branch, but of course it's not available yet until this PR is merged. |
return args | ||
|
||
|
||
def submits_lotus(compare_yml, config_user, dry_run=False): |
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.
We're not actually using config_user here, but I'm keeping it for completeness, just in case.
Okay @valeriupredoi, I'm happy with this now. |
all good by me too, bud! Go ahead and merge when you good 🍺 |
Closes #118
It's looking like this script is working now.
This PR adds a slurm queue based batch parallel processing of single job timeseries tool.
It's got the following features:
input_yaml
files.analysis_comparison
tool: the single job analysis.bgc
and one hasphysics
), it will only run the first one.Need to do: