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: make it compatible with the latest version of PBSPro #5910

Merged
merged 5 commits into from
Mar 9, 2023

Conversation

TurboKyle
Copy link
Contributor

Try to make it compatible with the latest version of PBS.

The nodes+ppn combination will be internally converted to the select+ncpus+mpiprocs pattern.
However, the select+ppn combination doesn't work on the latest PBS implementations. (Tested on OpenPBS 20.0.0)
Since I have a very limited test environment, I appreciate any of your feedback.

Thanks!

@TurboKyle TurboKyle changed the title make it compatible with the latest version of PBSPro Fix: make it compatible with the latest version of PBSPro Mar 1, 2023
@sphuber
Copy link
Contributor

sphuber commented Mar 3, 2023

Thanks @TurboKyle . I did some searching and it seems that the ppn notation has been deprecated for a while, but hasn't been removed for backwards compatibility. I tried to find a reference in the official documentation of PBSPro, but I can't find when it was deprecated or if/when it has been removed. Unfortunately, this makes it difficult to estimate whether the proposed change is breaking, since I don't know the probability that users might still be on older versions. @giovannipizzi what do you think?

@sphuber
Copy link
Contributor

sphuber commented Mar 3, 2023

Found a comment on an issue in another project that says the option was deprecated in PBSPro 13 which has been released years ago. Unfortunately the source of the quote is not included so cannot verify its validity. But it seems this change was made long ago then, so we may be able to safely change this

@TurboKyle
Copy link
Contributor Author

TurboKyle commented Mar 3, 2023

Thank you for your kind reply. @sphuber

This compatibility rule already exists in the BACKWARD COMPATIBILITY section of the pbs_resource man page for the version 14.1 branch, the earliest version visible on GitHub, for converting nodes:ppn combinations to select:ncpus:mpiprocs. I think we can guarantee compatibility at least from version PBS Pro 14.1.

If we want to support earlier versions, we may need to replace it with the nodes:ppn syntax. 14.1 onwards retains support for this syntax, but the syntax for specifying resources is different and probably more difficult to maintain.

I would like to see what you think.

@sphuber
Copy link
Contributor

sphuber commented Mar 9, 2023

Thanks @TurboKyle . Discussed it internally and we have decided to accept this change. I have updated the tests to make them pass.

@sphuber sphuber self-requested a review March 9, 2023 17:41
@sphuber sphuber merged commit 059ab66 into aiidateam:main Mar 9, 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.

2 participants