From a399d07099bba754dc1067f50b33775c0e88bdfc Mon Sep 17 00:00:00 2001 From: Andre Merzky Date: Tue, 14 Mar 2023 22:17:53 +0100 Subject: [PATCH 01/11] allow int env values in job spec --- src/psij/executors/batch/batch_scheduler_executor.py | 3 ++- src/psij/executors/local.py | 4 ++++ src/psij/job_executor.py | 8 ++++---- src/psij/job_spec.py | 5 +++-- tests/plugins1/_batch_test/test/test.mustache | 6 +++--- tests/test_doc_examples.py | 2 +- tests/test_executor.py | 7 ++++--- tests/test_job_spec.py | 11 +++++------ 8 files changed, 26 insertions(+), 20 deletions(-) diff --git a/src/psij/executors/batch/batch_scheduler_executor.py b/src/psij/executors/batch/batch_scheduler_executor.py index 5437bd55..1998a278 100644 --- a/src/psij/executors/batch/batch_scheduler_executor.py +++ b/src/psij/executors/batch/batch_scheduler_executor.py @@ -53,7 +53,8 @@ def _env_to_mustache(job: Job) -> List[Dict[str, str]]: r = [] for k, v in job.spec.environment.items(): - r.append({'name': k, 'value': bash_escape(v)}) + r.append({'name': k, 'value': bash_escape(str(v))}) + return r diff --git a/src/psij/executors/local.py b/src/psij/executors/local.py index 2ca241a9..cf7a1203 100644 --- a/src/psij/executors/local.py +++ b/src/psij/executors/local.py @@ -98,6 +98,10 @@ def _get_env(spec: JobSpec) -> Optional[Dict[str, str]]: # merge current env with spec env env = os.environ.copy() env.update(spec.environment) + # convert integers to strings + for k, v in env.items(): + if isinstance(v, int): + env[k] = str(v) return env else: # only spec env diff --git a/src/psij/job_executor.py b/src/psij/job_executor.py index ffe645f2..c1570d32 100644 --- a/src/psij/job_executor.py +++ b/src/psij/job_executor.py @@ -67,7 +67,7 @@ def _check_job(self, job: Job) -> JobSpec: Verifies that various aspects of the job are correctly specified. This includes precisely the following checks: * the job has a non-null specification - * job.spec.environment is a Dict[str, str] + * job.spec.environment is a Dict[str, [str | int]] While this method makes a fair attempt at ensuring the validity of the job, it makes no such guarantees. Specifically, if an executor implementation requires checks not listed @@ -101,9 +101,9 @@ def _check_job(self, job: Job) -> JobSpec: if not isinstance(k, str): raise TypeError('environment key "%s" is not a string (%s)' % (k, type(k).__name__)) - if not isinstance(v, str): - raise TypeError('environment key "%s" has non-string value (%s)' - % (k, type(v).__name__)) + if not isinstance(v, (str, int)): + raise TypeError('environment value for key "%s" must be string ' + 'or int type (%s)' % (k, type(v).__name__)) job.executor = self return spec diff --git a/src/psij/job_spec.py b/src/psij/job_spec.py index 18d17aa5..a97f92a4 100644 --- a/src/psij/job_spec.py +++ b/src/psij/job_spec.py @@ -1,6 +1,6 @@ import sys from pathlib import Path -from typing import Optional, List, Dict, Any +from typing import Optional, List, Dict, Any, Union from typeguard import check_argument_types @@ -15,7 +15,8 @@ class JobSpec(object): def __init__(self, name: Optional[str] = None, executable: Optional[str] = None, arguments: Optional[List[str]] = None, directory: Optional[Path] = None, - inherit_environment: bool = True, environment: Optional[Dict[str, str]] = None, + inherit_environment: bool = True, + environment: Optional[Dict[str, Union[str, int]]] = None, stdin_path: Optional[Path] = None, stdout_path: Optional[Path] = None, stderr_path: Optional[Path] = None, resources: Optional[ResourceSpec] = None, attributes: Optional[JobAttributes] = None, pre_launch: Optional[Path] = None, diff --git a/tests/plugins1/_batch_test/test/test.mustache b/tests/plugins1/_batch_test/test/test.mustache index 9d3bae37..e87b2396 100644 --- a/tests/plugins1/_batch_test/test/test.mustache +++ b/tests/plugins1/_batch_test/test/test.mustache @@ -22,8 +22,8 @@ export {{key}}="{{value}}" {{/custom_attributes.test}} {{/job.spec.attributes}} -{{#job.spec.inherit_environment}}env \{{/job.spec.inherit_environment}}{{^job.spec.inherit_environment}}env --ignore-environment \{{/job.spec.inherit_environment}}{{#env}} -{{name}}="{{value}}" \ +{{#job.spec.inherit_environment}}env \ +{{/job.spec.inherit_environment}}{{^job.spec.inherit_environment}}env --ignore-environment \{{/job.spec.inherit_environment}}{{#env}} {{name}}="{{value}}" \ {{/env}}{{#psij.launch_command}}{{.}} {{/psij.launch_command}} -echo "$?" > "{{psij.script_dir}}/$PSIJ_BATCH_TEST_JOB_ID.ec" \ No newline at end of file +echo "$?" > "{{psij.script_dir}}/$PSIJ_BATCH_TEST_JOB_ID.ec" diff --git a/tests/test_doc_examples.py b/tests/test_doc_examples.py index a94bd542..0d218299 100644 --- a/tests/test_doc_examples.py +++ b/tests/test_doc_examples.py @@ -112,7 +112,7 @@ def test_job_parameters() -> None: psij.JobSpec( executable="/bin/hostname", stdout_path=output_path, - environment={"FOOBAR": "BAZ"}, # custom environment has no effect here + environment={"FOOBAR": "BAZ", "BUZ": 1}, # custom environment has no effect here directory=pathlib.Path(td), # CWD has no effect on result here ) ) diff --git a/tests/test_executor.py b/tests/test_executor.py index 80e9a121..1e07b2a2 100644 --- a/tests/test_executor.py +++ b/tests/test_executor.py @@ -117,10 +117,11 @@ def test_env_var(execparams: ExecutorTestParams) -> None: _make_test_dir() with TemporaryDirectory(dir=Path.home() / '.psij' / 'test') as td: outp = Path(td, 'stdout.txt') - job = Job(JobSpec(executable='/bin/bash', arguments=['-c', 'echo -n $TEST_VAR'], + job = Job(JobSpec(executable='/bin/bash', + arguments=['-c', 'env > /tmp/t; echo -n $TEST_VAR$TEST_INT'], stdout_path=outp)) assert job.spec is not None - job.spec.environment = {'TEST_VAR': '_y_'} + job.spec.environment = {'TEST_INT': 1, 'TEST_VAR': '_y_'} ex = _get_executor_instance(execparams, job) ex.submit(job) status = job.wait(timeout=_get_timeout(execparams)) @@ -128,7 +129,7 @@ def test_env_var(execparams: ExecutorTestParams) -> None: f = outp.open("r") contents = f.read() f.close() - assert contents == '_y_' + assert contents == '_y_1' def test_stdin_redirect(execparams: ExecutorTestParams) -> None: diff --git a/tests/test_job_spec.py b/tests/test_job_spec.py index 3a072919..7fdc7a5d 100644 --- a/tests/test_job_spec.py +++ b/tests/test_job_spec.py @@ -10,16 +10,14 @@ def _test_spec(spec: JobSpec) -> None: def test_environment_types() -> None: - with pytest.raises(TypeError): - _test_spec(JobSpec(environment={'foo': 1})) # type: ignore with pytest.raises(TypeError): - _test_spec(JobSpec(environment={1: 'foo'})) # type: ignore + _test_spec(JobSpec(executable='true', environment={1: 'foo'})) # type: ignore with pytest.raises(TypeError): - spec = JobSpec() + spec = JobSpec(executable='true') spec.environment = {'foo': 'bar'} - spec.environment['buz'] = 2 # type: ignore + spec.environment['buz'] = [2] # type: ignore _test_spec(spec) spec = JobSpec() @@ -32,8 +30,9 @@ def test_environment_types() -> None: spec.environment = {'foo': 'bar'} assert spec.environment['foo'] == 'bar' - spec.environment = {'foo': 'biz'} + spec.environment = {'foo': 'biz', 'bar': 42} assert spec.environment['foo'] == 'biz' + assert spec.environment['bar'] == 42 test_environment_types() From 12211d68d050bcae67c7fdfbf0831cbad664912d Mon Sep 17 00:00:00 2001 From: Andre Merzky Date: Tue, 14 Mar 2023 22:42:03 +0100 Subject: [PATCH 02/11] convert env on setting --- .../batch/batch_scheduler_executor.py | 3 +-- src/psij/executors/local.py | 4 ---- src/psij/job_spec.py | 18 ++++++++++++++++++ tests/test_job_spec.py | 2 +- 4 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/psij/executors/batch/batch_scheduler_executor.py b/src/psij/executors/batch/batch_scheduler_executor.py index 1998a278..5437bd55 100644 --- a/src/psij/executors/batch/batch_scheduler_executor.py +++ b/src/psij/executors/batch/batch_scheduler_executor.py @@ -53,8 +53,7 @@ def _env_to_mustache(job: Job) -> List[Dict[str, str]]: r = [] for k, v in job.spec.environment.items(): - r.append({'name': k, 'value': bash_escape(str(v))}) - + r.append({'name': k, 'value': bash_escape(v)}) return r diff --git a/src/psij/executors/local.py b/src/psij/executors/local.py index cf7a1203..2ca241a9 100644 --- a/src/psij/executors/local.py +++ b/src/psij/executors/local.py @@ -98,10 +98,6 @@ def _get_env(spec: JobSpec) -> Optional[Dict[str, str]]: # merge current env with spec env env = os.environ.copy() env.update(spec.environment) - # convert integers to strings - for k, v in env.items(): - if isinstance(v, int): - env[k] = str(v) return env else: # only spec env diff --git a/src/psij/job_spec.py b/src/psij/job_spec.py index a97f92a4..125064bf 100644 --- a/src/psij/job_spec.py +++ b/src/psij/job_spec.py @@ -111,6 +111,24 @@ def _init_job_spec_dict(self) -> Dict[str, Any]: return job_spec + @property + def environment(self) -> Optional[Dict[str, str]]: + """Return the environment dict.""" + return self._environment + + @environment.setter + def environment(self, env: Optional[Dict[str, Union[str, int]]]) -> None: + """Ensure env dict values to be string typed.""" + self._environment = None + + if env: + self._environment = {} + for k, v in env.items(): + if isinstance(v, int): + self._environment[k] = str(v) + else: + self._environment[k] = v + @property def to_dict(self) -> Dict[str, Any]: """Returns a dictionary representation of this object.""" diff --git a/tests/test_job_spec.py b/tests/test_job_spec.py index 7fdc7a5d..fccf4c36 100644 --- a/tests/test_job_spec.py +++ b/tests/test_job_spec.py @@ -32,7 +32,7 @@ def test_environment_types() -> None: spec.environment = {'foo': 'biz', 'bar': 42} assert spec.environment['foo'] == 'biz' - assert spec.environment['bar'] == 42 + assert spec.environment['bar'] == '42' test_environment_types() From 92b9462e269648989af88ef5bdc61603ade6f7a9 Mon Sep 17 00:00:00 2001 From: Andre Merzky Date: Mon, 23 Oct 2023 17:09:11 +0200 Subject: [PATCH 03/11] env setter checks for int type --- src/psij/job_spec.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/psij/job_spec.py b/src/psij/job_spec.py index 1090bec3..e32b8f4c 100644 --- a/src/psij/job_spec.py +++ b/src/psij/job_spec.py @@ -170,6 +170,7 @@ def environment(self, env: Optional[Dict[str, Union[str, int]]]) -> None: else: self._environment[k] = v + @property def directory(self) -> Optional[pathlib.Path]: """The directory, on the compute side, in which the executable is to be run.""" return self._directory From 6d49995a8664787d4002801a573c4bf7436c448b Mon Sep 17 00:00:00 2001 From: Andre Merzky Date: Mon, 15 Jan 2024 15:41:55 +0100 Subject: [PATCH 04/11] batch executor now exports env --- tests/plugins1/_batch_test/test/test.mustache | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/plugins1/_batch_test/test/test.mustache b/tests/plugins1/_batch_test/test/test.mustache index 23a98671..8989f132 100644 --- a/tests/plugins1/_batch_test/test/test.mustache +++ b/tests/plugins1/_batch_test/test/test.mustache @@ -39,9 +39,7 @@ for NODE in $(seq 1 1 "$PSIJ_TEST_BATCH_EXEC_COUNT"); do done export PSIJ_NODEFILE - -{{#job.spec.inherit_environment}}env \{{/job.spec.inherit_environment}}{{^job.spec.inherit_environment}}env --ignore-environment \{{/job.spec.inherit_environment}}{{#env}} -{{name}}="{{value}}" \ -{{/env}}{{#psij.launch_command}}{{.}} {{/psij.launch_command}} +{{#job.spec.inherit_environment}}{{/job.spec.inherit_environment}}{{^job.spec.inherit_environment}}env --ignore-environment \{{/job.spec.inherit_environment}}{{#env}} +export {{name}}="{{value}}" {{/env}} echo "$?" > "{{psij.script_dir}}/$PSIJ_BATCH_TEST_JOB_ID.ec" From 1bf70ca3548e6b9174806f8cb33f4b519f84baaa Mon Sep 17 00:00:00 2001 From: Andre Merzky Date: Mon, 15 Jan 2024 16:06:36 +0100 Subject: [PATCH 05/11] try to make mypy happy --- src/psij/job_spec.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/psij/job_spec.py b/src/psij/job_spec.py index e32b8f4c..3694721a 100644 --- a/src/psij/job_spec.py +++ b/src/psij/job_spec.py @@ -153,7 +153,7 @@ def name(self, value: Optional[str]) -> None: self._name = value @property - def environment(self) -> Optional[Dict[str, Union[str, int]]]: + def environment(self) -> Optional[Dict[str, str]]: """Return the environment dict.""" return self._environment From 5a3349530f878bfc75378079838b14426f5dcfd3 Mon Sep 17 00:00:00 2001 From: Andre Merzky Date: Fri, 9 Feb 2024 16:51:27 +0100 Subject: [PATCH 06/11] apply suggested converter method also disable mypy checks for int/string env conversion tests --- src/psij/job_spec.py | 24 ++++++++++++++---------- tests/test_executor.py | 2 +- tests/test_job_spec.py | 2 +- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/psij/job_spec.py b/src/psij/job_spec.py index 3694721a..c69db715 100644 --- a/src/psij/job_spec.py +++ b/src/psij/job_spec.py @@ -21,6 +21,18 @@ def _to_path(arg: Union[str, pathlib.Path, None]) -> Optional[pathlib.Path]: return pathlib.Path(arg) +def _to_env_dict(arg: Union[Dict[str, Union[str, int]], None]) -> Dict[str, str]: + if arg is None: + return dict() + ret = dict() + for k,v in arg.items(): + if isinstance(v, int): + ret[k] = str(v) + else: + ret[k] = v + return ret + + class JobSpec(object): """A class that describes the details of a job.""" @@ -125,7 +137,7 @@ def __init__(self, executable: Optional[str] = None, arguments: Optional[List[st # care of the conversion, but mypy gets confused self._directory = _to_path(directory) self.inherit_environment = inherit_environment - self.environment = environment + self.environment = _to_env_dict(environment) self._stdin_path = _to_path(stdin_path) self._stdout_path = _to_path(stdout_path) self._stderr_path = _to_path(stderr_path) @@ -160,15 +172,7 @@ def environment(self) -> Optional[Dict[str, str]]: @environment.setter def environment(self, env: Optional[Dict[str, Union[str, int]]]) -> None: """Ensure env dict values to be string typed.""" - self._environment = None - - if env: - self._environment = {} - for k, v in env.items(): - if isinstance(v, int): - self._environment[k] = str(v) - else: - self._environment[k] = v + self._environment = _to_env_dict(env) @property def directory(self) -> Optional[pathlib.Path]: diff --git a/tests/test_executor.py b/tests/test_executor.py index 3180188c..e425a030 100644 --- a/tests/test_executor.py +++ b/tests/test_executor.py @@ -126,7 +126,7 @@ def test_env_var(execparams: ExecutorTestParams) -> None: arguments=['-c', 'env > /tmp/t; echo -n $TEST_VAR$TEST_INT'], stdout_path=outp)) assert job.spec is not None - job.spec.environment = {'TEST_INT': 1, 'TEST_VAR': '_y_'} + job.spec.environment = {'TEST_INT': 1, 'TEST_VAR': '_y_'} # type: ignore ex = _get_executor_instance(execparams, job) ex.submit(job) status = job.wait(timeout=_get_timeout(execparams)) diff --git a/tests/test_job_spec.py b/tests/test_job_spec.py index d91fc8bb..9fa57a49 100644 --- a/tests/test_job_spec.py +++ b/tests/test_job_spec.py @@ -32,7 +32,7 @@ def test_environment_types() -> None: spec.environment = {'foo': 'bar'} assert spec.environment['foo'] == 'bar' - spec.environment = {'foo': 'biz', 'bar': 42} + spec.environment = {'foo': 'biz', 'bar': 42} # type: ignore assert spec.environment['foo'] == 'biz' assert spec.environment['bar'] == '42' From e6fd370935ac26322b657234720483a2c75e2812 Mon Sep 17 00:00:00 2001 From: Andre Merzky Date: Fri, 9 Feb 2024 16:57:09 +0100 Subject: [PATCH 07/11] linting --- src/psij/job_spec.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/psij/job_spec.py b/src/psij/job_spec.py index c69db715..003eb8af 100644 --- a/src/psij/job_spec.py +++ b/src/psij/job_spec.py @@ -25,7 +25,7 @@ def _to_env_dict(arg: Union[Dict[str, Union[str, int]], None]) -> Dict[str, str] if arg is None: return dict() ret = dict() - for k,v in arg.items(): + for k, v in arg.items(): if isinstance(v, int): ret[k] = str(v) else: @@ -41,7 +41,8 @@ def __init__(self, executable: Optional[str] = None, arguments: Optional[List[st # sphinx fails to find the class. Using Path in the getters and setters does not # appear to trigger a problem. directory: Union[str, pathlib.Path, None] = None, name: Optional[str] = None, - inherit_environment: bool = True, environment: Optional[Dict[str, Union[str, int]]] = None, + inherit_environment: bool = True, + environment: Optional[Dict[str, Union[str, int]]] = None, stdin_path: Union[str, pathlib.Path, None] = None, stdout_path: Union[str, pathlib.Path, None] = None, stderr_path: Union[str, pathlib.Path, None] = None, From cbdeea905b6a70cb0e6c50b706d1b057abf9981c Mon Sep 17 00:00:00 2001 From: Andre Merzky Date: Fri, 9 Feb 2024 17:06:28 +0100 Subject: [PATCH 08/11] recover previous behavior on empty env --- src/psij/job_spec.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/psij/job_spec.py b/src/psij/job_spec.py index 003eb8af..b2f34359 100644 --- a/src/psij/job_spec.py +++ b/src/psij/job_spec.py @@ -23,7 +23,7 @@ def _to_path(arg: Union[str, pathlib.Path, None]) -> Optional[pathlib.Path]: def _to_env_dict(arg: Union[Dict[str, Union[str, int]], None]) -> Dict[str, str]: if arg is None: - return dict() + return None ret = dict() for k, v in arg.items(): if isinstance(v, int): From 1c4e5114335a5fa6ba155558318552b46278354d Mon Sep 17 00:00:00 2001 From: Andre Merzky Date: Fri, 16 Feb 2024 09:58:12 +0100 Subject: [PATCH 09/11] optional dict... --- src/psij/job_spec.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/psij/job_spec.py b/src/psij/job_spec.py index b2f34359..7eec03ec 100644 --- a/src/psij/job_spec.py +++ b/src/psij/job_spec.py @@ -21,7 +21,7 @@ def _to_path(arg: Union[str, pathlib.Path, None]) -> Optional[pathlib.Path]: return pathlib.Path(arg) -def _to_env_dict(arg: Union[Dict[str, Union[str, int]], None]) -> Dict[str, str]: +def _to_env_dict(arg: Union[Dict[str, Union[str, int]], None]) -> Optional[Dict[str, str]]: if arg is None: return None ret = dict() From 1a46f26307a43188d36f424441d24ff105b8309e Mon Sep 17 00:00:00 2001 From: Andre Merzky Date: Fri, 16 Feb 2024 15:01:13 +0100 Subject: [PATCH 10/11] revert non-fix --- tests/plugins1/_batch_test/test/test.mustache | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/plugins1/_batch_test/test/test.mustache b/tests/plugins1/_batch_test/test/test.mustache index 53f9bd07..245e3ba8 100644 --- a/tests/plugins1/_batch_test/test/test.mustache +++ b/tests/plugins1/_batch_test/test/test.mustache @@ -41,7 +41,9 @@ for NODE in $(seq 1 1 "$PSIJ_TEST_BATCH_EXEC_COUNT"); do done export PSIJ_NODEFILE -{{#job.spec.inherit_environment}}{{/job.spec.inherit_environment}}{{^job.spec.inherit_environment}}env --ignore-environment \{{/job.spec.inherit_environment}}{{#env}} -export {{name}}="{{value}}" {{/env}} -echo "$?" > "{{psij.script_dir}}/$PSIJ_BATCH_TEST_JOB_ID.ec" +{{#job.spec.inherit_environment}}env \{{/job.spec.inherit_environment}}{{^job.spec.inherit_environment}}env --ignore-environment \{{/job.spec.inherit_environment}}{{#env}} +{{name}}="{{value}}" \ +{{/env}}{{#psij.launch_command}}{{.}} {{/psij.launch_command}} + +echo "$?" > "{{psij.script_dir}}/$PSIJ_BATCH_TEST_JOB_ID.ec" \ No newline at end of file From 88bda656f6974832c6ed60fc6815322541fd402a Mon Sep 17 00:00:00 2001 From: Andre Merzky Date: Wed, 21 Feb 2024 17:40:46 +0100 Subject: [PATCH 11/11] better fix for multiple env vars in test executor --- tests/plugins1/_batch_test/test/test.mustache | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/plugins1/_batch_test/test/test.mustache b/tests/plugins1/_batch_test/test/test.mustache index 245e3ba8..ae51ae68 100644 --- a/tests/plugins1/_batch_test/test/test.mustache +++ b/tests/plugins1/_batch_test/test/test.mustache @@ -43,7 +43,7 @@ done export PSIJ_NODEFILE {{#job.spec.inherit_environment}}env \{{/job.spec.inherit_environment}}{{^job.spec.inherit_environment}}env --ignore-environment \{{/job.spec.inherit_environment}}{{#env}} -{{name}}="{{value}}" \ -{{/env}}{{#psij.launch_command}}{{.}} {{/psij.launch_command}} +{{name}}="{{value}}" \{{/env}} +{{#psij.launch_command}}{{.}} {{/psij.launch_command}} -echo "$?" > "{{psij.script_dir}}/$PSIJ_BATCH_TEST_JOB_ID.ec" \ No newline at end of file +echo "$?" > "{{psij.script_dir}}/$PSIJ_BATCH_TEST_JOB_ID.ec"