feat: support slurm 26.05.x - #427
Conversation
|
Hey, I'll have a look at it :) Just one note: Usually I merged into main directly from a throwaway branch, and then afterwards create the 26.x.x branches, so I can easily sync them with main via rebase. When this is merged, a new merge commit is probably created, diverging main and 26.05.x. Would probably be better if this PR is merged via rebase |
There was a problem hiding this comment.
Thanks for tackling the update to 26.05.
All the changes in pyslurm/* look reasonable, that seems fine.
Hopefully I'm not sounding too harsh, but what I really dislike is how the new tests added here are written (besides test_get_resource_layout_per_node maybe).
I realize this must be clearly AI generated, because most of the tests seem nonsensical and don't really actually test what should be tested.
For example, all the new unit tests in test_job.py testing exclusive and oversubscribe is technically correct, but besides the point. If the API shapes up and really properly turns things that should be Enums into enums, then scaling this for every enum is a nightmare. Instead, there should be a dummy enum inheriting from SlurmEnum and such, that simply tests if all the functionality works as expected.
If it does, then it can be safely assumed it works for every other thing inheriting from SlurmEnum - no need to test everything individually.
Same for all the explicit tests for the new members in tests/integration/test_job.py. These tests are missing the point.
For example test_suspend_time. Retrieving suspend_time uses u32_parse under the hood, which is already tested in test_common.py. With the comment, it looks like the AI wants to make sure that all the C-Fields are in order when generated by autopxd2. Valid concern, but then the shape of the actual C-API and the generated one should be verified in a different way and once before comitting, not by targeting a random member of a struct.
Also the test_new_26_06_config_members test: While again technically correct checking if the returned type is actually a list/bool, this is bogus. When converting a comma-separated string from Slurm, cstr.to_list is usually used. This does definitely return a list, and this functionality is already tested in test_common.py in TestString.test_lists(). If anything, the assert isinstance() should be added there, once.
What my point is without bashing every single newly added test line: While the tests seem technically correct, they are not testing what actually needs testing - which is the underlying functions/structs that are used to implement these new members, like cstr.to_list, cstr.fmalloc (for strings) or SlurmEnum, where most of them are already tested.
I know I haven't done any substantial contribution in the past months while being mostly silent and just now coming out to criticize. But I am grateful that you are continuing things, updating versions, adding a proper CI/CD and such. I don't really have the time and motivation at the moment
However, if AI is really going to be used on the project, I would appreciate still if anything the AI outputs is carefully reviewed, because these tests are really just not good and should be written entirely different or removed. If testing is contiuously generated by AI like this, the test-suite will become unmaintainable
| return cstr.to_unicode(slurm_job_share_string(self.ptr.shared)) | ||
|
|
||
| @property | ||
| def exclusive(self): |
There was a problem hiding this comment.
Both exclusive and oversubscribe supersede the legacy self.ptr.shared value. I'd suggest deprecating the resource_sharing member, because it will be removed upstream in the future.
No description provided.