Skip to content

Add some missing native specifications#9

Merged
natefoo merged 8 commits intonatefoo:masterfrom
nsoranzo:add_missing_native_specs
May 3, 2018
Merged

Add some missing native specifications#9
natefoo merged 8 commits intonatefoo:masterfrom
nsoranzo:add_missing_native_specs

Conversation

@nsoranzo
Copy link
Copy Markdown
Contributor

See commits for details.

@natefoo
Copy link
Copy Markdown
Owner

natefoo commented Apr 13, 2018

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
Copy link
Copy Markdown
Contributor Author

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

@ljyanesm
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Owner

natefoo commented Apr 16, 2018

@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
Copy link
Copy Markdown
Contributor Author

@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
Copy link
Copy Markdown
Owner

natefoo commented Apr 16, 2018

@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
Copy link
Copy Markdown
Owner

natefoo commented May 2, 2018

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
Copy link
Copy Markdown
Contributor Author

nsoranzo commented May 2, 2018

@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.

@natefoo natefoo force-pushed the add_missing_native_specs branch from 1d58232 to fe78d17 Compare May 2, 2018 21:01
Copy link
Copy Markdown
Contributor Author

@nsoranzo nsoranzo left a comment

Choose a reason for hiding this comment

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

Thanks @natefoo for the follow up!

@natefoo natefoo merged commit 44cc67e into natefoo:master May 3, 2018
@nsoranzo nsoranzo deleted the add_missing_native_specs branch May 4, 2018 10:55
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.

3 participants