From dac018bfc82e047bf3766d91abe1378b95e481de Mon Sep 17 00:00:00 2001 From: Andreas Eknes Lie Date: Thu, 7 Sep 2023 10:22:28 +0200 Subject: [PATCH 1/5] Simplification of local_driver --- src/clib/lib/job_queue/local_driver.cpp | 84 +++++++++++++------------ 1 file changed, 43 insertions(+), 41 deletions(-) diff --git a/src/clib/lib/job_queue/local_driver.cpp b/src/clib/lib/job_queue/local_driver.cpp index 730f226867f..e8b1d839c39 100644 --- a/src/clib/lib/job_queue/local_driver.cpp +++ b/src/clib/lib/job_queue/local_driver.cpp @@ -1,45 +1,37 @@ #include #include -#include - #include #include #include - -#include +#include #include #include #include +#include typedef struct local_job_struct local_job_type; struct local_job_struct { - bool active; - job_status_type status; - std::optional run_thread; - pid_t child_process; + bool active = false; + job_status_type status = JOB_QUEUE_WAITING; + std::optional run_thread = std::nullopt; + pid_t child_process = 0; }; struct local_driver_struct { std::mutex submit_lock; }; -static local_job_type *local_job_alloc() { - local_job_type *job = new local_job_type; - job->active = false; - job->status = JOB_QUEUE_WAITING; - return job; -} +static local_job_type *local_job_alloc() { return new local_job_type; } -job_status_type local_driver_get_job_status(void *__driver, void *__job) { - if (__job == NULL) - /* The job has not been registered at all ... */ - return JOB_QUEUE_NOT_ACTIVE; - else { +job_status_type local_driver_get_job_status(void * /**__driver*/, void *__job) { + if (__job != nullptr) { local_job_type *job = reinterpret_cast(__job); return job->status; } + + return JOB_QUEUE_NOT_ACTIVE; // The job has not been registered at all } void local_driver_free_job(void *__job) { @@ -48,7 +40,7 @@ void local_driver_free_job(void *__job) { free(job); } -void local_driver_kill_job(void *__driver, void *__job) { +void local_driver_kill_job(void * /**__driver*/, void *__job) { local_job_type *job = reinterpret_cast(__job); if (job->child_process > 0) kill(job->child_process, SIGTERM); @@ -63,37 +55,33 @@ void submit_job_thread(const char *executable, int argc, char **argv, local_job_type *job) { int wait_status; job->child_process = - spawn(executable, argc, (const char **)argv, NULL, NULL); + spawn(executable, argc, (const char **)argv, nullptr, nullptr); util_free_stringlist(argv, argc); waitpid(job->child_process, &wait_status, 0); job->active = false; job->status = JOB_QUEUE_EXIT; - if (WIFEXITED(wait_status)) - if (WEXITSTATUS(wait_status) == 0) - job->status = JOB_QUEUE_DONE; + if (WIFEXITED(wait_status) != 0 && (WEXITSTATUS(wait_status) == 0)) + job->status = JOB_QUEUE_DONE; } void *local_driver_submit_job(void *__driver, const char *submit_cmd, - int num_cpu, /* Ignored */ - const char *run_path, const char *job_name, - int argc, const char **argv) { + int /** num_cpu */, const char * /**run_path*/, + const char * /**job_name*/, int argc, + const char **argv) { local_driver_type *driver = reinterpret_cast(__driver); - { - local_job_type *job = local_job_alloc(); + local_job_type *job = local_job_alloc(); + auto argv_copy = util_alloc_stringlist_copy(argv, argc); - auto argv_copy = util_alloc_stringlist_copy(argv, argc); + std::lock_guard guard{driver->submit_lock}; + job->active = true; + job->status = JOB_QUEUE_RUNNING; - std::lock_guard guard{driver->submit_lock}; - job->active = true; - job->status = JOB_QUEUE_RUNNING; + job->run_thread = std::thread{ + [=] { submit_job_thread(submit_cmd, argc, argv_copy, job); }}; + job->run_thread->detach(); - job->run_thread = std::thread{ - [=] { submit_job_thread(submit_cmd, argc, argv_copy, job); }}; - job->run_thread->detach(); - - return job; - } + return job; } void local_driver_free(local_driver_type *driver) { delete driver; } @@ -105,6 +93,20 @@ void local_driver_free__(void *__driver) { void *local_driver_alloc() { return new local_driver_type; } -void local_driver_init_option_list(stringlist_type *option_list) { - //No options specific for local driver; do nothing +void local_driver_init_option_list(stringlist_type * /**option_list*/) {} + +bool local_driver_set_option(void * /**__driver*/, const char * /**option_key*/, + const void * /**value_*/) { + util_abort( + "%s: Local driver does not support run time setting of options\n", + __func__); + return false; +} + +const void *local_driver_get_option(const void * /**__driver*/, + const char * /**option_key*/) { + util_abort( + "%s: Local driver does not support run time reading of options\n", + __func__); + return nullptr; } From 3551a9a0103bc47fa1ca232fbd805b607f0b96d5 Mon Sep 17 00:00:00 2001 From: Andreas Eknes Lie Date: Thu, 7 Sep 2023 10:47:25 +0200 Subject: [PATCH 2/5] Add get-set option functions to localdriver --- .../include/ert/job_queue/local_driver.hpp | 5 ++- src/clib/lib/job_queue/queue_driver.cpp | 33 +++---------------- 2 files changed, 9 insertions(+), 29 deletions(-) diff --git a/src/clib/lib/include/ert/job_queue/local_driver.hpp b/src/clib/lib/include/ert/job_queue/local_driver.hpp index 5a8ee17df7c..7a6a4cd2bea 100644 --- a/src/clib/lib/include/ert/job_queue/local_driver.hpp +++ b/src/clib/lib/include/ert/job_queue/local_driver.hpp @@ -16,5 +16,8 @@ void local_driver_free__(void *__driver); job_status_type local_driver_get_job_status(void *__driver, void *__job); void local_driver_free_job(void *__job); void local_driver_init_option_list(stringlist_type *option_list); - +bool local_driver_set_option(void *__driver, const char *option_key, + const void *value_); +const void *local_driver_get_option(const void *__driver, + const char *option_key); #endif diff --git a/src/clib/lib/job_queue/queue_driver.cpp b/src/clib/lib/job_queue/queue_driver.cpp index 0add2086f60..272ff2f3c73 100644 --- a/src/clib/lib/job_queue/queue_driver.cpp +++ b/src/clib/lib/job_queue/queue_driver.cpp @@ -58,21 +58,9 @@ const char *queue_driver_get_name(const queue_driver_type *driver) { return driver->name; } -/** - Set option - can also be used to perform actions - not only setting - of parameters. There is no limit :-) - */ bool queue_driver_set_option(queue_driver_type *driver, const char *option_key, const void *value) { - if (driver->set_option != NULL) - /* The actual low level set functions can not fail! */ - return driver->set_option(driver->data, option_key, value); - else { - util_abort( - "%s: driver:%s does not support run time setting of options\n", - __func__, driver->name); - return false; - } + return driver->set_option(driver->data, option_key, value); } /** @@ -126,6 +114,8 @@ queue_driver_type *queue_driver_alloc(job_driver_type type) { driver->kill_job = local_driver_kill_job; driver->free_job = local_driver_free_job; driver->free_driver = local_driver_free__; + driver->set_option = local_driver_set_option; + driver->get_option = local_driver_get_option; driver->name = util_alloc_string_copy("local"); driver->init_options = local_driver_init_option_list; driver->data = local_driver_alloc(); @@ -163,25 +153,12 @@ queue_driver_type *queue_driver_alloc(job_driver_type type) { const void *queue_driver_get_option(queue_driver_type *driver, const char *option_key) { - if (driver->get_option != NULL) - /* The actual low level set functions can not fail! */ - return driver->get_option(driver->data, option_key); - else { - util_abort( - "%s: driver:%s does not support run time reading of options\n", - __func__, driver->name); - return NULL; - } + return driver->get_option(driver->data, option_key); } void queue_driver_init_option_list(queue_driver_type *driver, stringlist_type *option_list) { - if (driver->init_options) - driver->init_options(option_list); - else - util_abort( - "%s: driver:%s does not support run time reading of options\n", - __func__, driver->name); + driver->init_options(option_list); } /* These are the functions used by the job_queue layer. */ From 959a1396727990f2f265727234e1371ac825870d Mon Sep 17 00:00:00 2001 From: Andreas Eknes Lie Date: Thu, 7 Sep 2023 13:15:46 +0200 Subject: [PATCH 3/5] Lift driver name to python --- .../lib/include/ert/job_queue/queue_driver.hpp | 4 ---- src/clib/lib/job_queue/queue_driver.cpp | 14 -------------- .../old_tests/job_queue/test_job_queue_driver.cpp | 5 ----- src/ert/job_queue/driver.py | 4 ++-- tests/unit_tests/job_queue/test_driver.py | 11 +++++++++++ 5 files changed, 13 insertions(+), 25 deletions(-) diff --git a/src/clib/lib/include/ert/job_queue/queue_driver.hpp b/src/clib/lib/include/ert/job_queue/queue_driver.hpp index 3aec82dce55..328817e5bdf 100644 --- a/src/clib/lib/include/ert/job_queue/queue_driver.hpp +++ b/src/clib/lib/include/ert/job_queue/queue_driver.hpp @@ -38,10 +38,6 @@ void queue_driver_free_job(queue_driver_type *driver, void *job_data); void queue_driver_kill_job(queue_driver_type *driver, void *job_data); job_status_type queue_driver_get_status(queue_driver_type *driver, void *job_data); - -extern "C" PY_USED const char * -queue_driver_get_name(const queue_driver_type *driver); - extern "C" bool queue_driver_set_option(queue_driver_type *driver, const char *option_key, const void *value); diff --git a/src/clib/lib/job_queue/queue_driver.cpp b/src/clib/lib/job_queue/queue_driver.cpp index 272ff2f3c73..81e8001b9dc 100644 --- a/src/clib/lib/job_queue/queue_driver.cpp +++ b/src/clib/lib/job_queue/queue_driver.cpp @@ -48,16 +48,8 @@ struct queue_driver_struct { /** Driver specific data - passed as first argument to the driver functions above. */ void *data; - - /* Generic data - common to all driver types. */ - /** String name of driver. */ - char *name; }; -const char *queue_driver_get_name(const queue_driver_type *driver) { - return driver->name; -} - bool queue_driver_set_option(queue_driver_type *driver, const char *option_key, const void *value) { return driver->set_option(driver->data, option_key, value); @@ -81,7 +73,6 @@ static queue_driver_type *queue_driver_alloc_empty() { driver->free_driver = NULL; driver->get_option = NULL; driver->set_option = NULL; - driver->name = NULL; driver->data = NULL; driver->init_options = NULL; @@ -104,7 +95,6 @@ queue_driver_type *queue_driver_alloc(job_driver_type type) { driver->free_driver = lsf_driver_free__; driver->set_option = lsf_driver_set_option; driver->get_option = lsf_driver_get_option; - driver->name = util_alloc_string_copy("LSF"); driver->init_options = lsf_driver_init_option_list; driver->data = lsf_driver_alloc(); break; @@ -116,7 +106,6 @@ queue_driver_type *queue_driver_alloc(job_driver_type type) { driver->free_driver = local_driver_free__; driver->set_option = local_driver_set_option; driver->get_option = local_driver_get_option; - driver->name = util_alloc_string_copy("local"); driver->init_options = local_driver_init_option_list; driver->data = local_driver_alloc(); break; @@ -128,12 +117,10 @@ queue_driver_type *queue_driver_alloc(job_driver_type type) { driver->free_driver = torque_driver_free__; driver->set_option = torque_driver_set_option; driver->get_option = torque_driver_get_option; - driver->name = util_alloc_string_copy("TORQUE"); driver->init_options = torque_driver_init_option_list; driver->data = torque_driver_alloc(); break; case SLURM_DRIVER: - driver->name = util_alloc_string_copy("SLURM"); driver->set_option = slurm_driver_set_option; driver->get_option = slurm_driver_get_option; driver->init_options = slurm_driver_init_option_list; @@ -191,6 +178,5 @@ void queue_driver_free_driver(queue_driver_type *driver) { void queue_driver_free(queue_driver_type *driver) { queue_driver_free_driver(driver); - free(driver->name); delete driver; } diff --git a/src/clib/old_tests/job_queue/test_job_queue_driver.cpp b/src/clib/old_tests/job_queue/test_job_queue_driver.cpp index 3365c6522fb..78e052190a2 100644 --- a/src/clib/old_tests/job_queue/test_job_queue_driver.cpp +++ b/src/clib/old_tests/job_queue/test_job_queue_driver.cpp @@ -47,8 +47,6 @@ void get_driver_option_lists() { //Torque driver option list { queue_driver_type *driver_torque = queue_driver_alloc(TORQUE_DRIVER); - test_assert_string_equal(queue_driver_get_name(driver_torque), - "TORQUE"); stringlist_type *option_list = stringlist_alloc_new(); queue_driver_init_option_list(driver_torque, option_list); @@ -62,7 +60,6 @@ void get_driver_option_lists() { //Local driver option list (only general queue_driver options) { queue_driver_type *driver_local = queue_driver_alloc(LOCAL_DRIVER); - test_assert_string_equal(queue_driver_get_name(driver_local), "local"); stringlist_type *option_list = stringlist_alloc_new(); queue_driver_init_option_list(driver_local, option_list); @@ -73,7 +70,6 @@ void get_driver_option_lists() { //Lsf driver option list { queue_driver_type *driver_lsf = queue_driver_alloc(LSF_DRIVER); - test_assert_string_equal(queue_driver_get_name(driver_lsf), "LSF"); stringlist_type *option_list = stringlist_alloc_new(); queue_driver_init_option_list(driver_lsf, option_list); @@ -88,7 +84,6 @@ void get_driver_option_lists() { //SLurm driver option list { queue_driver_type *driver_slurm = queue_driver_alloc(SLURM_DRIVER); - test_assert_string_equal(queue_driver_get_name(driver_slurm), "SLURM"); stringlist_type *option_list = stringlist_alloc_new(); queue_driver_init_option_list(driver_slurm, option_list); diff --git a/src/ert/job_queue/driver.py b/src/ert/job_queue/driver.py index 948c0df9b01..e5ea804037b 100644 --- a/src/ert/job_queue/driver.py +++ b/src/ert/job_queue/driver.py @@ -13,7 +13,6 @@ class Driver(BaseCClass): # type: ignore _free = ResPrototype("void queue_driver_free( driver )") _set_option = ResPrototype("void queue_driver_set_option( driver , char* , char*)") _get_option = ResPrototype("char* queue_driver_get_option(driver, char*)") - _get_name = ResPrototype("char* queue_driver_get_name( driver )") def __init__( self, @@ -23,6 +22,7 @@ def __init__( c_ptr = self._alloc(driver_type) super().__init__(c_ptr) self._max_running = 0 + self._driver_name = driver_type.name if options: for key, value in options: @@ -66,7 +66,7 @@ def create_driver(cls, queue_config: QueueConfig) -> "Driver": @property def name(self) -> str: - return self._get_name() + return self._driver_name def free(self) -> None: self._free() diff --git a/tests/unit_tests/job_queue/test_driver.py b/tests/unit_tests/job_queue/test_driver.py index fb4ab2b19fb..5e7ce0720f7 100644 --- a/tests/unit_tests/job_queue/test_driver.py +++ b/tests/unit_tests/job_queue/test_driver.py @@ -24,6 +24,17 @@ def test_set_and_unset_option(): assert driver.get_option("MAX_RUNNING") == "0" +def test_get_driver_name(): + queue_config = QueueConfig(queue_system=QueueSystem.LOCAL) + assert Driver.create_driver(queue_config).name == "LOCAL" + queue_config = QueueConfig(queue_system=QueueSystem.SLURM) + assert Driver.create_driver(queue_config).name == "SLURM" + queue_config = QueueConfig(queue_system=QueueSystem.TORQUE) + assert Driver.create_driver(queue_config).name == "TORQUE" + queue_config = QueueConfig(queue_system=QueueSystem.LSF) + assert Driver.create_driver(queue_config).name == "LSF" + + def test_get_slurm_queue_config(): queue_config = QueueConfig( job_script=os.path.abspath("script.sh"), From d29e9c14df43760c4da2ab4ebae2a7075fa9e2ff Mon Sep 17 00:00:00 2001 From: Andreas Eknes Lie Date: Mon, 11 Sep 2023 09:11:31 +0200 Subject: [PATCH 4/5] Add test for local_driver get-set util abort --- .../job_queue/test_job_queue_driver.cpp | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/clib/old_tests/job_queue/test_job_queue_driver.cpp b/src/clib/old_tests/job_queue/test_job_queue_driver.cpp index 78e052190a2..9261d7126e1 100644 --- a/src/clib/old_tests/job_queue/test_job_queue_driver.cpp +++ b/src/clib/old_tests/job_queue/test_job_queue_driver.cpp @@ -4,8 +4,9 @@ #include #include +#include #include - +#include #include #include @@ -63,6 +64,22 @@ void get_driver_option_lists() { stringlist_type *option_list = stringlist_alloc_new(); queue_driver_init_option_list(driver_local, option_list); + test_assert_util_abort( + "local_driver_get_option", + [](void *arg) { + auto local_driver = static_cast(arg); + queue_driver_get_option(local_driver, "NA"); + }, + driver_local); + + test_assert_util_abort( + "local_driver_set_option", + [](void *arg) { + auto local_driver = static_cast(arg); + queue_driver_set_option(local_driver, "NA", "NA"); + }, + driver_local); + stringlist_free(option_list); queue_driver_free(driver_local); } @@ -97,6 +114,7 @@ void get_driver_option_lists() { } int main(int argc, char **argv) { + util_install_signals(); job_queue_set_driver_(LSF_DRIVER); job_queue_set_driver_(LOCAL_DRIVER); job_queue_set_driver_(TORQUE_DRIVER); From 265f1c45d4ae7e4d890f85bfbed87ceeb17fd70b Mon Sep 17 00:00:00 2001 From: Andreas Eknes Lie Date: Mon, 11 Sep 2023 10:39:03 +0200 Subject: [PATCH 5/5] Refactor driver options tests --- .../job_queue/test_job_queue_driver.cpp | 105 +++++++----------- 1 file changed, 39 insertions(+), 66 deletions(-) diff --git a/src/clib/old_tests/job_queue/test_job_queue_driver.cpp b/src/clib/old_tests/job_queue/test_job_queue_driver.cpp index 9261d7126e1..faad47ac342 100644 --- a/src/clib/old_tests/job_queue/test_job_queue_driver.cpp +++ b/src/clib/old_tests/job_queue/test_job_queue_driver.cpp @@ -1,7 +1,7 @@ -#include - #include #include +#include +#include #include #include @@ -44,73 +44,41 @@ void set_option_valid_on_specific_driver_returns_true() { queue_driver_free(driver_torque); } -void get_driver_option_lists() { - //Torque driver option list - { - queue_driver_type *driver_torque = queue_driver_alloc(TORQUE_DRIVER); - stringlist_type *option_list = stringlist_alloc_new(); - queue_driver_init_option_list(driver_torque, option_list); - - for (const auto &i : TORQUE_DRIVER_OPTIONS) { - test_assert_true(stringlist_contains(option_list, i.c_str())); - } - stringlist_free(option_list); - queue_driver_free(driver_torque); - } +void get_driver_option_lists(job_driver_type driver_type, + std::vector driver_options) { + queue_driver_type *driver_ = queue_driver_alloc(driver_type); + stringlist_type *option_list = stringlist_alloc_new(); + queue_driver_init_option_list(driver_, option_list); - //Local driver option list (only general queue_driver options) - { - queue_driver_type *driver_local = queue_driver_alloc(LOCAL_DRIVER); - stringlist_type *option_list = stringlist_alloc_new(); - queue_driver_init_option_list(driver_local, option_list); - - test_assert_util_abort( - "local_driver_get_option", - [](void *arg) { - auto local_driver = static_cast(arg); - queue_driver_get_option(local_driver, "NA"); - }, - driver_local); - - test_assert_util_abort( - "local_driver_set_option", - [](void *arg) { - auto local_driver = static_cast(arg); - queue_driver_set_option(local_driver, "NA", "NA"); - }, - driver_local); - - stringlist_free(option_list); - queue_driver_free(driver_local); + for (const auto &i : driver_options) { + test_assert_true(stringlist_contains(option_list, i.c_str())); } - //Lsf driver option list - { - queue_driver_type *driver_lsf = queue_driver_alloc(LSF_DRIVER); - stringlist_type *option_list = stringlist_alloc_new(); - queue_driver_init_option_list(driver_lsf, option_list); - - for (const auto &i : LSF_DRIVER_OPTIONS) { - test_assert_true(stringlist_contains(option_list, i.c_str())); - } - - stringlist_free(option_list); - queue_driver_free(driver_lsf); - } - - //SLurm driver option list - { - queue_driver_type *driver_slurm = queue_driver_alloc(SLURM_DRIVER); - stringlist_type *option_list = stringlist_alloc_new(); - queue_driver_init_option_list(driver_slurm, option_list); - - for (const auto &i : SLURM_DRIVER_OPTIONS) { - test_assert_true(stringlist_contains(option_list, i.c_str())); - } + stringlist_free(option_list); + queue_driver_free(driver_); +} - stringlist_free(option_list); - queue_driver_free(driver_slurm); - } +void test_local_driver_no_get_set_options() { + queue_driver_type *driver_local = queue_driver_alloc(LOCAL_DRIVER); + stringlist_type *option_list = stringlist_alloc_new(); + queue_driver_init_option_list(driver_local, option_list); + test_assert_util_abort( + "local_driver_get_option", + [](void *arg) { + auto local_driver = static_cast(arg); + queue_driver_get_option(local_driver, "NA"); + }, + driver_local); + + test_assert_util_abort( + "local_driver_set_option", + [](void *arg) { + auto local_driver = static_cast(arg); + queue_driver_set_option(local_driver, "NA", "NA"); + }, + driver_local); + stringlist_free(option_list); + queue_driver_free(driver_local); } int main(int argc, char **argv) { @@ -124,7 +92,12 @@ int main(int argc, char **argv) { set_option_invalid_value_returns_false(); set_option_valid_on_specific_driver_returns_true(); - get_driver_option_lists(); + + get_driver_option_lists(TORQUE_DRIVER, TORQUE_DRIVER_OPTIONS); + get_driver_option_lists(SLURM_DRIVER, SLURM_DRIVER_OPTIONS); + get_driver_option_lists(LSF_DRIVER, LSF_DRIVER_OPTIONS); + + test_local_driver_no_get_set_options(); exit(0); }