issues 153 and 150 #156
issues 153 and 150 #156
Conversation
…en slurmdb_job_rec_t in slurm and pyslurm which were fixed up
|
Thank you for the contribution, @l1ll1 🚀 This looks great. Would you be able to write tests for this change to ensure this continues to work going forward? Also, it's ok if some struct members aren't re-declared if they are not used. |
|
Thanks @giovtorres regarding the struct members, would you like me to remove them? I've never worked in Cython and didn't realize it wasn't necessary to declare all members of the struct, so I found it a bit confusing ;-). I can cancel the PR and clean up the code base if that helps? |
It's ok, you can leave them in for completeness. Having them in won't affect the behaviour. |
|
Hmm, it doesn't look like these tests executed. |
tests/test-slurmdb.py
Outdated
| return len(jobs) | ||
|
|
||
| def get_user(): | ||
| import pwd |
There was a problem hiding this comment.
Let's move this import to the top of the file
tests/test-slurmdb.py
Outdated
| endtime = (datetime.datetime.now()-datetime.timedelta(days=1)).strftime("%Y-%m-%dT00:00:00") | ||
| njobs_pyslurm = njobs_slurmdb_jobs_get(starttime,endtime) | ||
| njobs_sacct = njobs_sacct_jobs(starttime,endtime) | ||
| assert_equals(njobs_pyslurm,njobs_sacct) |
tests/test-slurmdb.py
Outdated
| njobs_pyslurm = njobs_slurmdb_jobs_get_byuid(starttime,endtime,int(user[1])) | ||
| njobs_sacct = njobs_sacct_jobs_byuser(starttime,endtime,user[0]) | ||
| print('njobs by sacct {}'.format(njobs_sacct)) | ||
| assert_equals(njobs_pyslurm,njobs_sacct) |
| @@ -0,0 +1,54 @@ | |||
| from __future__ import absolute_import, unicode_literals | |||
|
|
|||
| import pyslurm | |||
There was a problem hiding this comment.
I would reorder these as Python builtin modules, 3rd party modules and custom modules, all separated by a space.
There's some opportunity for refactoring and removing some code duplication. Otherwise, logic is good. |
tests/test-slurmdb.py
Outdated
|
|
||
|
|
||
| def test_slurmdb_jobs_get(): | ||
| starttime = (datetime.datetime.now()-datetime.timedelta(days=2)).strftime("%Y-%m-%dT00:00:00") |
There was a problem hiding this comment.
Could you add a doctstring here similar to the other test methods in this suite?
| assert_equals(njobs_pyslurm,njobs_sacct) | ||
|
|
||
| def test_slurmdb_jobs_get_byuser(): | ||
| userlist = list(get_user()) |
There was a problem hiding this comment.
Same here.... Could you add a doctstring here similar to the other test methods in this suite?
| def njobs_sacct_jobs(start, end): | ||
| sacct = subprocess.Popen(['sacct','-S',start,'-E',end,'-n','-X','-a'],stdout=subprocess.PIPE,stderr=None).communicate() | ||
| return len(sacct[0].splitlines()) | ||
|
|
There was a problem hiding this comment.
There should be two spaces between methods. Same applies below...
|
Thanks @giovtorres ... feeling suitably embarrassed: perhaps one day I'll get around to turning on pep8 checking right in vim and I won't submit such a dogs breakfast for PRs anymore ;-) |
| @@ -1,54 +1,81 @@ | |||
| from __future__ import absolute_import, unicode_literals | |||
| import datetime | |||
| import pwd | |||
giovtorres
left a comment
giovtorres
left a comment
There was a problem hiding this comment.
Thank you for addressing feedback!
2 participants
Hi,
I believe this PR fixes issues 153 (slurmdb_jobs.get missing jobs) and 150 (slurmdb_jobs.get to take a list of users).
Apparently job_cond.db_flags needs to default to SLURMDB_JOB_FLAG_NOTSET (found this in the sacct source code)
There appear some to be some missing fields on the slurmdb_job_rec_t which I fixed as well.
The userids need to be either the uidNumber as a string or the uidNumber as an int. Left as an exercise for the user to convert usernames to uid Numbers.
Happy to review the MR and add documentation if necessary.
Thanks for a great tool.