search

natefoo / slurm-drmaa

DRMAA for Slurm: Implementation of the DRMAA C bindings for Slurm
GNU General Public License v3.0
48 stars 22 forks source link

Add some missing native specifications #9

Closed nsoranzo closed 6 years ago

nsoranzo commented 6 years ago

See commits for details.

natefoo commented 6 years ago

Thanks @nsoranzo for this submission and sorry I didn't see it until you pointed it out!

Only one concern here, I suspect that the reason the original slurm-drmaa authors did not include the stdout and stderr native options is that the drmaa_output_path and drmaa_error_path job attributes are the "proper" DRMAA way to set these. Native specifications themselves are something of a hack (but a very necessary hack, especially for DRMAA v1) to allow for users to utilize DRM-specific features even when consuming the DRMAA.

Is there a reason why using the native options for these would be preferable to the job attributes?

nsoranzo commented 6 years ago

@natefoo I actually got this part of the code from my colleague @ljyanesm, he may have a better idea.

ljyanesm commented 6 years ago

Hi guys,

I am happy to edit the changes to use drmaa_output_path/error_path , I used native specifications since it was the first working solution I found.

Would you want this PR to be updated as advised before merge?

natefoo commented 6 years ago

@ljyanesm I think that'd be the right solution. Similarly, the job name is also a DRMAA job template parameter so this should not be supported in the native specification. The rest looks great. Thanks!

nsoranzo commented 6 years ago

@natefoo This PR only add documentation for -J, --job-name=*name*, the support was already there in slurm_drmaa/util.c , should we remove the added docs?

natefoo commented 6 years ago

@nsoranzo Whoops. Interesting that it was already supported, I hadn't noticed that. Removing the documentation would be ok, or at least adding to the documentation a note that the DRMAA job template job name is the preferred method. From a quick glance at the code it looks like the DRMAA name would override the native spec name, if both are set.

natefoo commented 6 years ago

I'm going to make a new release shortly, it'd be great to include this work.

On further reflection, I guess there is little harm in having these options available and documented, but I guess the only request I'd make is to document them in a separate section that explains that they duplicate the DRMAA attributes, and the DRMAA attributes are preferred.

nsoranzo commented 6 years ago

@natefoo Thanks! I can't promise I'll be able to work on this in the next couple of days, feel free to commit to my branch if you are in a hurry.