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

Do not skip job_extra_directives with header_skip. Rename header_skp. #584

Merged
merged 2 commits into from
Sep 3, 2022

Conversation

guillaumeeb
Copy link
Member

Fixes #461.

I didn't thought I would make so many changes for this one...

But I thought I'd need to rename header_skip to be more in line with last changes.
And then I also run into problems with SGECluster header that was not created as the other one.
There is also HTCondorCluster wihch is a special case.

In the end, I prefer the code this way, but I would be happy to have some comments, maybe from @jolange, @riedel or @mivade who have been a bit active here lately.

Also keep in mind that at one point this code might completely change if we finally implement #338.

dask_jobqueue/sge.py Show resolved Hide resolved
dask_jobqueue/core.py Outdated Show resolved Hide resolved
dask_jobqueue/core.py Outdated Show resolved Hide resolved
@riedel
Copy link
Member

riedel commented Sep 2, 2022

I actually never used this, however, I really find it a bit strange to use because you really need to know what the code will generate to then suppress it (The question is how to know if "#SBATCH -p" or "#SBATCH --partition=" so it implicitly fixes contract far below API level)

You also will get weird side effects eg. with header_skip=["-N"] if eg. the queue is named "Queue-New" (I guess it would be a valid queue name) or a jobname contains a minus something.

Then again what is the use case? Is there any variable I cannot simply set to none to supress the generation of default directives?
I found something here:

https://github.com/dask/dask-blog/blob/58a93bceed4be9fbfb215e1be5200f0dcbce866a/_posts/2019-08-28-dask-on-summit.md?plain=1#L143

Which is the thing I do not understand: why would you first set the memory requested and then remove it from the request.

If I would want to support such hacking I would rather just support a generic post_process script that you can pass the whole submit file through if you really like to do weird stuff.

After making such a probably unsubstantial rant: your pull request actually makes things only better :)

@guillaumeeb
Copy link
Member Author

Thanks for your valuable comment @riedel!

I agree this is a really advanced feature, but I've seen it used a few times, not only by Matthew Rocklin on Summit cluster. It's clearly a bit hacky, and hopefully, we'll make things better at some point if we implement #338.

Anyway, I'll stick happily with your final line:

After making such a probably unsubstantial rant: your pull request actually makes things only better :)

@guillaumeeb guillaumeeb merged commit 1167281 into dask:main Sep 3, 2022
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.

SLURMCluster: header_skip should not skip user-defined extra header lines
2 participants