From 2424e0569eb724cc08a1a79f5615183c3207c4ca Mon Sep 17 00:00:00 2001 From: Mark Lugar <14322382+araglu@users.noreply.github.com> Date: Mon, 22 Apr 2024 17:52:36 -0500 Subject: [PATCH] Fix errors with unit tests (#54) * Change some tests with an imaginary HPC The "44" value caused errors in another piece of software. "192" would fix it for the replacement HPC, but I went with "1" to hopefully work longer than 4 years. * Change directive tests to use real PBS settings -J is used for job arrays, and -A is the subproject number. Now it uses -l for application tracking and -o for stdout filename. * Fix narwhal ncpus npcus were 44 which were for Onyx. Narwhal has 128. Rename pbs_gpu to pbs_script everywhere. * Replace onyx with carpenter * Change uit.uit. and uit.uit.yaml imports for pytest to work Also fix a str concat error with NoneType. * Fix under-indentation from renaming pbs_gpu --- tests/test_pbs_script.py | 117 +++++++++++++++++++-------------------- tests/test_uit.py | 36 ++++++------ uit/testing_utils.py | 12 ++-- uit/uit.py | 2 +- 4 files changed, 81 insertions(+), 86 deletions(-) diff --git a/tests/test_pbs_script.py b/tests/test_pbs_script.py index d968695..dd04b67 100644 --- a/tests/test_pbs_script.py +++ b/tests/test_pbs_script.py @@ -81,14 +81,14 @@ def test_job_array_indices(self): def test_set_directive(self): # Call the method - self.pbs.set_directive('-J', 'OepnGL') + self.pbs.set_directive('-l', 'application=other') # Get the list of namedtuple ret = self.pbs._optional_directives # Test the results - self.assertEqual('-J', ret[0].directive) - self.assertEqual('OepnGL', ret[0].options) + self.assertEqual('-l', ret[0].directive) + self.assertEqual('application=other', ret[0].options) def test_get_directive(self): self.pbs.set_directive('-A', 'C++') @@ -117,15 +117,15 @@ def test_get_directive_default(self): self.assertEqual('test', res) def test_get_directives(self): - self.pbs.set_directive('-J', 'OepnGL') - self.pbs.set_directive('-A', 'C++') + self.pbs.set_directive('-l', 'application=other') + self.pbs.set_directive('-o', 'stdout.log') # Call the method res = self.pbs.optional_directives # Test the result - self.assertEqual('OepnGL', res[0].options) - self.assertEqual('C++', res[1].options) + self.assertEqual('application=other', res[0].options) + self.assertEqual('stdout.log', res[1].options) def test_module_use(self): self.pbs.module_use('path') @@ -146,82 +146,77 @@ def test_render_required_directives_block_not_None(self, mock_job_array_directiv self.assertIn('this is fake', res) def test_get_render_required_directives_block_for_narwhal_compute_node(self): - res = self.pbs.render_required_directives_block() - self.assertIn('#PBS -N ' + self.pbs.name, res) - self.assertIn('#PBS -A ' + self.pbs.project_id, res) - self.assertIn("#PBS -q " + self.pbs.queue, res) - self.assertIn(f'#PBS -l select={self.pbs.num_nodes}:ncpus=44:mpiprocs={self.pbs.processes_per_node}', res) + pbs_script = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=1, max_time="20:30:30", + node_type='compute', system='narwhal') + res = pbs_script.render_required_directives_block() + self.assertIn('#PBS -N ' + pbs_script.name, res) + self.assertIn('#PBS -A ' + pbs_script.project_id, res) + self.assertIn("#PBS -q " + pbs_script.queue, res) + self.assertIn(f'#PBS -l select={pbs_script.num_nodes}:' + f'ncpus={NODE_TYPES["narwhal"]["compute"]}:' + f'mpiprocs={pbs_script.processes_per_node}', res) def test_get_render_required_directives_block_for_narwhal_gpu_node(self): - pbs_gpu = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=1, max_time="20:30:30", - node_type='gpu', system='narwhal') - res = pbs_gpu.render_required_directives_block() - self.assertIn(f'#PBS -l select={pbs_gpu.num_nodes}:ncpus={NODE_TYPES["narwhal"]["compute"]}:' - f'mpiprocs={pbs_gpu.processes_per_node}', res) + pbs_script = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=1, max_time="20:30:30", + node_type='gpu', system='narwhal') + res = pbs_script.render_required_directives_block() + self.assertIn(f'#PBS -l select={pbs_script.num_nodes}:ncpus={NODE_TYPES["narwhal"]["compute"]}:' + f'mpiprocs={pbs_script.processes_per_node}', res) def test_get_render_required_directives_block_for_narwhal_bigmem_node(self): - pbs_gpu = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=1, max_time="20:30:30", - node_type='bigmem', system='narwhal') - res = pbs_gpu.render_required_directives_block() - self.assertIn(f'#PBS -l select={pbs_gpu.num_nodes}:ncpus={NODE_TYPES["narwhal"]["bigmem"]}:' - f'mpiprocs={pbs_gpu.processes_per_node}', res) + pbs_script = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=1, max_time="20:30:30", + node_type='bigmem', system='narwhal') + res = pbs_script.render_required_directives_block() + self.assertIn(f'#PBS -l select={pbs_script.num_nodes}:ncpus={NODE_TYPES["narwhal"]["bigmem"]}:' + f'mpiprocs={pbs_script.processes_per_node}', res) def test_get_render_required_directives_block_for_narwhal_transfer_node(self): - pbs_gpu = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=1, max_time="20:30:30", - node_type='transfer', system='narwhal') - res = pbs_gpu.render_required_directives_block() + pbs_script = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=1, max_time="20:30:30", + node_type='transfer', system='narwhal') + res = pbs_script.render_required_directives_block() self.assertIn('#PBS -l select=5:ncpus=1', res) - def test_get_render_required_directives_block_for_onyx_compute_node(self): - pbs_gpu = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=11, max_time="20:30:30", - node_type='compute', system='onyx') - expected = f'#PBS -l select={pbs_gpu.num_nodes}:ncpus={NODE_TYPES["onyx"]["compute"]}:' \ - f'mpiprocs={pbs_gpu.processes_per_node}' + def test_get_render_required_directives_block_for_carpenter_compute_node(self): + pbs_script = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=24, max_time="20:30:30", + node_type='compute', system='carpenter') + expected = f'#PBS -l select={pbs_script.num_nodes}:ncpus={NODE_TYPES["carpenter"]["compute"]}:' \ + f'mpiprocs={pbs_script.processes_per_node}' - res = pbs_gpu.render_required_directives_block() + res = pbs_script.render_required_directives_block() self.assertIn(expected, res) - def test_get_render_required_directives_block_for_onyx_gpu_node(self): - pbs_gpu = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=11, max_time="20:30:30", - node_type='gpu', system='onyx') - expected = f'#PBS -l select={pbs_gpu.num_nodes}:ncpus={NODE_TYPES["onyx"]["gpu"]}:' \ - f'mpiprocs={pbs_gpu.processes_per_node}:ngpus=1' + def test_get_render_required_directives_block_for_carpenter_gpu_node(self): + pbs_script = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=32, max_time="20:30:30", + node_type='gpu', system='carpenter') + expected = f'#PBS -l select={pbs_script.num_nodes}:ncpus={NODE_TYPES["carpenter"]["gpu"]}:' \ + f'mpiprocs={pbs_script.processes_per_node}:ngpus=1' - res = pbs_gpu.render_required_directives_block() + res = pbs_script.render_required_directives_block() self.assertIn(expected, res) - def test_get_render_required_directives_block_for_onyx_bigmem_node(self): - pbs_gpu = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=11, max_time="20:30:30", - node_type='bigmem', system='onyx') - expected = f'#PBS -l select={pbs_gpu.num_nodes}:ncpus={NODE_TYPES["onyx"]["bigmem"]}:' \ - f'mpiprocs={pbs_gpu.processes_per_node}:bigmem=1' + def test_get_render_required_directives_block_for_carpenter_bigmem_node(self): + pbs_script = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=24, max_time="20:30:30", + node_type='bigmem', system='carpenter') + expected = f'#PBS -l select={pbs_script.num_nodes}:ncpus={NODE_TYPES["carpenter"]["bigmem"]}:' \ + f'mpiprocs={pbs_script.processes_per_node}:bigmem=1' - res = pbs_gpu.render_required_directives_block() + res = pbs_script.render_required_directives_block() self.assertIn(expected, res) - def test_get_render_required_directives_block_for_onyx_transfer_node(self): - pbs_gpu = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=1, max_time="20:30:30", - node_type='transfer', system='onyx') - res = pbs_gpu.render_required_directives_block() + def test_get_render_required_directives_block_for_carpenter_transfer_node(self): + pbs_script = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=1, max_time="20:30:30", + node_type='transfer', system='carpenter') + res = pbs_script.render_required_directives_block() self.assertIn('PBS -l select=5:ncpus=1', res) - def test_get_render_required_directives_block_for_onyx_knl_node(self): - pbs_gpu = PbsScript(name='test1', project_id='P001', num_nodes=5, processes_per_node=1, max_time="20:30:30", - node_type='knl', system='onyx') - expected = f'#PBS -l select={pbs_gpu.num_nodes}:ncpus={NODE_TYPES["onyx"]["knl"]}:' \ - f'mpiprocs={pbs_gpu.processes_per_node}:nmics=1' - - res = pbs_gpu.render_required_directives_block() - self.assertIn(expected, res) - def test_get_render_optional_directives_block(self): self.pbs.set_directive('-A', 'ADH') - self.pbs.set_directive('-J', 'OpenMP') + self.pbs.set_directive('-o', 'stdout.log') res = self.pbs.render_optional_directives_block() self.assertIn('#PBS -A ADH', res) - self.assertIn('#PBS -J OpenMP', res) + self.assertIn('#PBS -o stdout.log', res) def test_load_module(self): # load anaconda module @@ -280,7 +275,7 @@ def test_render(self): self.pbs.swap_module('Anaconda', 'OpenMP') - self.pbs.set_directive('-J', 'OpenMP') + self.pbs.set_directive('-o', 'stdout.log') self.pbs.set_directive('-T', 'OpenGL') @@ -291,12 +286,12 @@ def test_render(self): self.assertIn("#PBS -A P001", render_str) self.assertIn("#PBS -N test1", render_str) self.assertIn("#PBS -q debug", render_str) - self.assertIn("#PBS -l select=5:ncpus=44:mpiprocs=1", render_str) + self.assertIn("#PBS -l select=5:ncpus=192:mpiprocs=1", render_str) self.assertIn("#PBS -l walltime=20:30:30", render_str) self.assertIn('module load C++', render_str) self.assertIn('module unload OpenGL', render_str) self.assertIn('module swap Anaconda OpenMP', render_str) - self.assertIn('#PBS -J OpenMP', render_str) + self.assertIn('#PBS -o stdout.log', render_str) self.assertIn('#PBS -T OpenGL', render_str) @mock.patch('uit.pbs_script.PbsScript.render') diff --git a/tests/test_uit.py b/tests/test_uit.py index eaa0d06..f84ed46 100644 --- a/tests/test_uit.py +++ b/tests/test_uit.py @@ -1,14 +1,14 @@ import unittest from unittest import mock from pathlib import PurePosixPath -from uit.uit import Client +from uit import Client class TestUIT(unittest.TestCase): def setUp(self): - from uit.uit import Client - with mock.patch('uit.uit.Client.get_userinfo') as _: + from uit import Client + with mock.patch('uit.Client.get_userinfo') as _: self.client = Client(token='test_token') self.client.connected = True @@ -33,17 +33,17 @@ def mock_import(name, *args): builtins.__import__ = real_import self.assertEqual(uit_test.has_pandas, False) - @mock.patch('uit.uit.open') - @mock.patch('uit.uit.yaml.safe_load') + @mock.patch('uit.config.open') + @mock.patch('uit.config.yaml.safe_load') def test_init_no_token(self, mock_yaml, _): mock_yaml.return_value = { 'client_id': 'client_id', 'client_secret': 'client_secret' } - Client() + Client(config_file='test') - @mock.patch('uit.uit.open') - @mock.patch('uit.uit.yaml.safe_load') + @mock.patch('uit.config.open') + @mock.patch('uit.config.yaml.safe_load') def test_init_no_credentials(self, mock_yaml, _): mock_yaml.return_value = {} self.assertRaises(ValueError, Client) @@ -54,25 +54,25 @@ def test_ensure_connected(self): RuntimeError, self.client.call, 'cmd' ) - @mock.patch('uit.uit.Client') + @mock.patch('uit.Client') def test_HOME(self, _): self.client.env.HOME = 'HOME env' res = self.client.HOME self.assertEqual(PurePosixPath('HOME env'), res) - @mock.patch('uit.uit.Client') + @mock.patch('uit.Client') def test_WORKDIR(self, _): self.client.env.WORKDIR = 'WORKDIR' res = self.client.WORKDIR self.assertEqual(PurePosixPath('WORKDIR'), res) - @mock.patch('uit.uit.Client') + @mock.patch('uit.Client') def test_WORKDIR2(self, _): self.client.env.WORKDIR2 = 'WORKDIR2' res = self.client.WORKDIR2 self.assertEqual(PurePosixPath('WORKDIR2'), res) - @mock.patch('uit.uit.Client') + @mock.patch('uit.Client') def test_CENTER(self, _): self.client.env.CENTER = 'CENTER' res = self.client.CENTER @@ -108,8 +108,8 @@ def test_userinfo(self): def test_username(self): self.assertEqual(None, self.client.username) - @mock.patch('uit.uit.Client.call') - @mock.patch('uit.uit.Client.put_file') + @mock.patch('uit.Client.call') + @mock.patch('uit.Client.put_file') def test_submit(self, _, mock_call): mock_call.return_value = 'J001' @@ -117,16 +117,16 @@ def test_submit(self, _, mock_call): self.assertEqual('J001', ret) - @mock.patch('uit.uit.Client.call') - @mock.patch('uit.uit.Client.put_file') + @mock.patch('uit.Client.call') + @mock.patch('uit.Client.put_file') def test_submit_runtime_error(self, mock_put_file, mock_call): mock_put_file.return_value = {'success': 'false', 'error': 'test_error'} mock_call.return_value = 'J001' self.assertRaises(RuntimeError, self.client.submit, pbs_script='test_script.sh', working_dir='\\test\\workdir') - @mock.patch('uit.uit.Client.call') - @mock.patch('uit.uit.Client.put_file') + @mock.patch('uit.Client.call') + @mock.patch('uit.Client.put_file') def test_submit_call_error(self, _, mock_call): mock_call.side_effect = RuntimeError diff --git a/uit/testing_utils.py b/uit/testing_utils.py index 7452e30..d7f9af7 100644 --- a/uit/testing_utils.py +++ b/uit/testing_utils.py @@ -7,7 +7,7 @@ class MockClient(Client): def __init__(self, *args, **kwargs): super().__init__(token='mock_token') - self.connect('onyx') + self.connect('testhpc') @property def WORKDIR(self): @@ -23,12 +23,12 @@ def get_userinfo(self): self._userinfo = { 'USERNAME': 'mock_user', 'SYSTEMS': { - 'ONYX': { + 'TESTHPC': { 'LOGIN_NODES': [ { - 'HOSTNAME': 'onyx01', + 'HOSTNAME': 'testhpc01', 'USERNAME': 'mock_user', - 'URLS': {'UIT': 'onyx01@mock.gov'} + 'URLS': {'UIT': 'testhpc01@mock.gov'} }, ] } @@ -106,7 +106,7 @@ def list_dir(self, path=None, parse=True, as_df=False): name='mock_script', project_id='mock_project_id', num_nodes=1, - processes_per_node=44, + processes_per_node=1, max_time='00:00:01' ) @@ -116,7 +116,7 @@ def list_dir(self, path=None, parse=True, as_df=False): name='mock_script', project_id='mock_project_id', num_nodes=1, - processes_per_node=44, + processes_per_node=1, max_time='00:00:01', array_indices=(0, 2) ) diff --git a/uit/uit.py b/uit/uit.py index b3437d0..7837409 100644 --- a/uit/uit.py +++ b/uit/uit.py @@ -110,7 +110,7 @@ def __init__(self, ca_file=None, config_file=None, client_id=None, client_secret if (self.client_id is None or self.client_secret is None) and self.token is None: raise ValueError('Please provide either the client_id and client_secret as kwargs, environment vars ' - '(UIT_ID, UIT_SECRET) or in auth config file: ' + config_file + ' OR provide an ' + '(UIT_ID, UIT_SECRET) or in auth config file: ' + str(config_file) + ' OR provide an ' 'access token as a kwarg.') if session_id is None: