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

Fix name for ntasks in slurm template #19

Merged
merged 2 commits into from
Jul 14, 2023

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Jul 14, 2023

Hopefully closes #18

@davidwaroquiers
Copy link
Member

Thanks, this is indeed a bug.

Maybe @gpetretto can comment on this (I don't fully remember why it is done this way but I remember we discussed about it), but I think the best would probably to modify it in the _convert_qresources method (https://github.com/Matgenix/QToolKit/blob/develop/src/qtoolkit/io/slurm.py#L540) at lines https://github.com/Matgenix/QToolKit/blob/develop/src/qtoolkit/io/slurm.py#L560, setting header_dict["ntasks"] = processes instead of header_dict["number_of_tasks"] = processes.

Note that I think https://github.com/Matgenix/QToolKit/blob/develop/src/qtoolkit/io/slurm.py#L564 should also be changed to header_dict["nodes"] = nodes.

@ml-evs
Copy link
Member Author

ml-evs commented Jul 14, 2023

I can confirm that making this change allows my slurm job through, can rename however you see fit

@davidwaroquiers
Copy link
Member

Now I think I remember. It is done this way so that when a user that knows slurm well want to directly pass a dictionary, he can use the "real" slurm variable.

@gpetretto
Copy link
Contributor

Indeed, the value in the template used to be number_of_tasks, but we decided that the names in the template should preferably match the original names, otherwise one needs to learn one more convention. I probably changed the template after writing the converter for QResources and forgot to change that point. Thanks for fixing this.

@davidwaroquiers davidwaroquiers merged commit fbb2060 into Matgenix:develop Jul 14, 2023
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.

number_of_tasks missing in slurm template
3 participants