-
Notifications
You must be signed in to change notification settings - Fork 125
Add ntasks and cpus_per_task options, fix db_conn in slurmdb_jobs #146
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
Conversation
pyslurm/pyslurm.pyx
Outdated
|
|
||
| def __dealloc__(self): | ||
| pass | ||
| self.__free() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since __free() is "private" and not used elsewhere, I think we can move the xfree and slurmdb_connection_close directly under __dealloc__.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no problem. i intended to follow the structure of some of the other classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done ;)
|
Thank you for this contribution! I do need to drop support for Python 2.6. I tried about year ago or so and some folks asked me to hold off, but I think it's time. Also, Python 2.7 support is going away on Jan 1 and may projects have pledged to drop support for Python 2. I'm considering the same for PySlurm. Python 3 support has been working for over a year now. I want to drop support for that CentOS 6 container altogether. |
| # desc.min_cpus = job_opts.get("ntasks") | ||
|
|
||
| # TODO: ntasks_set, cpus_set | ||
| if job_opts.get("overcommit"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we work on some tests for these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
submit_batch_job does not seem to be tested beyond giving a wrap script and a job_name. I added simple tests for ntasks and cpus_per_task...
| test_job_id = pyslurm.job().submit_batch_job(test_job) | ||
| test_job_search = pyslurm.job().find(name="name", val="pyslurm_test_job") | ||
| assert_true(test_job_id in test_job_search) | ||
| assert_equals(test_job_search["cpus_per_task"], 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
This PR builds on top of #141
It includes:
submit_batch_job: "ntasks", "cpus_per_task", "min_cpus", "max_cpus", "min_nodes", max_nodes":submit_batch_jobmethod is along the lines of the_fill_job_desc_from_optsfunction insrc/sbatch/sbatch.cof the Slurm source. Accordingly I adjusted the options involving "ntasks_set" and "cpus_set" which are both pretty much set along with "ntasks" and "cpus_per_task" in the Slurm source.slurmdb_jobs:db_connand close when theslurmdb_jobsinstance is freed.job_condwhich has been allocated in__cinit__.@giovtorres Regarding the Python 2.6 tests: These seem to fail because of a missing "debug" partition in the CentOS 6 docker containers (newer Python versions use CentOS 7). Couldn't you just drop official Python 2.6 support?