From 5704de15743046f6b7c2ffd40ade9b7a8a671c62 Mon Sep 17 00:00:00 2001 From: tcezard Date: Thu, 3 Oct 2024 12:22:41 +0100 Subject: [PATCH 1/4] Ensure that the a submission has been initiated before starting the upload Restart a submission's when it is marked as FAILED --- eva_sub_cli/orchestrator.py | 9 +++++++-- eva_sub_cli/submit.py | 13 +++++++++++++ tests/test_orchestrator.py | 24 +++++++++++++++--------- tests/test_submit.py | 23 +++++++++++++++++++++++ 4 files changed, 58 insertions(+), 11 deletions(-) diff --git a/eva_sub_cli/orchestrator.py b/eva_sub_cli/orchestrator.py index c4fb4f1..534c4ea 100755 --- a/eva_sub_cli/orchestrator.py +++ b/eva_sub_cli/orchestrator.py @@ -13,7 +13,8 @@ from eva_sub_cli.exceptions.submission_not_found_exception import SubmissionNotFoundException from eva_sub_cli.exceptions.submission_status_exception import SubmissionStatusException from eva_sub_cli.submission_ws import SubmissionWSClient -from eva_sub_cli.submit import StudySubmitter, SUB_CLI_CONFIG_KEY_SUBMISSION_ID +from eva_sub_cli.submit import StudySubmitter, SUB_CLI_CONFIG_KEY_SUBMISSION_ID, \ + SUB_CLI_CONFIG_KEY_SUBMISSION_UPLOAD_URL from eva_sub_cli.validators.docker_validator import DockerValidator from eva_sub_cli.validators.native_validator import NativeValidator from eva_sub_cli.validators.validator import READY_FOR_SUBMISSION_TO_EVA @@ -131,11 +132,15 @@ def check_validation_required(tasks, sub_config, username=None, password=None): if SUBMIT in tasks: if not sub_config.get(READY_FOR_SUBMISSION_TO_EVA, False): return True + # If we are working with an existing submission check its status to see if it was submitted and failed before. submission_id = sub_config.get(SUB_CLI_CONFIG_KEY_SUBMISSION_ID, None) if submission_id: try: submission_status = SubmissionWSClient(username, password).get_submission_status(submission_id) if submission_status == 'FAILED': + # Reset the submission_id which will force the creation of a new one + sub_config.set(SUB_CLI_CONFIG_KEY_SUBMISSION_ID, value=None) + sub_config.set(SUB_CLI_CONFIG_KEY_SUBMISSION_UPLOAD_URL, value=None) return True else: return False @@ -151,7 +156,7 @@ def check_validation_required(tasks, sub_config, username=None, password=None): raise SubmissionStatusException(f'Error occurred while getting status of the submission ' f'with Id {submission_id}') - logger.info(f'submission id not found in config. This might be the first time user is submitting') + logger.debug(f'submission id not found in config. This might be the first time user is submitting') return False diff --git a/eva_sub_cli/submit.py b/eva_sub_cli/submit.py index 4b7842c..15ce14c 100644 --- a/eva_sub_cli/submit.py +++ b/eva_sub_cli/submit.py @@ -81,6 +81,13 @@ def _complete_submission(self): # update config with completion of the submission self.sub_config.set(SUB_CLI_CONFIG_KEY_COMPLETE, value=True) + def is_submission_status_open(self): + submission_id = self.sub_config.get(SUB_CLI_CONFIG_KEY_SUBMISSION_ID) + if submission_id: + status = self.submission_ws_client.get_submission_status(submission_id) + return status == 'OPEN' + return False + def submit(self): if READY_FOR_SUBMISSION_TO_EVA not in self.sub_config or not self.sub_config[READY_FOR_SUBMISSION_TO_EVA]: raise Exception(f'There are still validation errors that need to be addressed. ' @@ -89,6 +96,12 @@ def submit(self): self.info(f'Initiate submission') self._initiate_submission() + if not self.is_submission_status_open(): + self.warning(f'You requested the submission using {self.submission_dir}. ' + f'This contains directory an already completed submission. ' + f'Please create a new submission directory to resubmit.') + return + # upload submission self.info(f'Upload data') self._upload_submission() diff --git a/tests/test_orchestrator.py b/tests/test_orchestrator.py index 22e6ec1..c033bc3 100644 --- a/tests/test_orchestrator.py +++ b/tests/test_orchestrator.py @@ -11,7 +11,7 @@ from eva_sub_cli.exceptions.submission_not_found_exception import SubmissionNotFoundException from eva_sub_cli.exceptions.submission_status_exception import SubmissionStatusException from eva_sub_cli.orchestrator import orchestrate_process, VALIDATE, SUBMIT, DOCKER, check_validation_required -from eva_sub_cli.submit import SUB_CLI_CONFIG_KEY_SUBMISSION_ID +from eva_sub_cli.submit import SUB_CLI_CONFIG_KEY_SUBMISSION_ID, SUB_CLI_CONFIG_KEY_SUBMISSION_UPLOAD_URL from eva_sub_cli.validators.validator import READY_FOR_SUBMISSION_TO_EVA from tests.test_utils import touch @@ -47,30 +47,36 @@ def tearDown(self) -> None: def test_check_validation_required(self): tasks = ['submit'] - sub_config = {READY_FOR_SUBMISSION_TO_EVA: False} + sub_config = WritableConfig(self.test_sub_dir, 'config.yaml') + sub_config.set(READY_FOR_SUBMISSION_TO_EVA, value=False) self.assertTrue(check_validation_required(tasks, sub_config)) - sub_config = {READY_FOR_SUBMISSION_TO_EVA: True} + sub_config.set(READY_FOR_SUBMISSION_TO_EVA, value=True) + self.assertFalse(check_validation_required(tasks, sub_config)) with patch('eva_sub_cli.submission_ws.SubmissionWSClient.get_submission_status') as get_submission_status_mock: - sub_config = {READY_FOR_SUBMISSION_TO_EVA: True, - SUB_CLI_CONFIG_KEY_SUBMISSION_ID: 'test123'} + sub_config.set(READY_FOR_SUBMISSION_TO_EVA, value=True) + sub_config.set(SUB_CLI_CONFIG_KEY_SUBMISSION_ID, value='test123') get_submission_status_mock.return_value = 'OPEN' self.assertFalse(check_validation_required(tasks, sub_config)) get_submission_status_mock.return_value = 'FAILED' self.assertTrue(check_validation_required(tasks, sub_config)) + # A FAILD submission status reset the submission ID and submission URL + self.assertEqual(sub_config.get(SUB_CLI_CONFIG_KEY_SUBMISSION_ID), None) + self.assertEqual(sub_config.get(SUB_CLI_CONFIG_KEY_SUBMISSION_UPLOAD_URL), None) + + sub_config.set(READY_FOR_SUBMISSION_TO_EVA, value=True) + sub_config.set(SUB_CLI_CONFIG_KEY_SUBMISSION_ID, value='test123') - mock_response = Mock() - mock_response.status_code = 404 + mock_response = Mock(status_code=404) get_submission_status_mock.side_effect = HTTPError(response=mock_response) with self.assertRaises(SubmissionNotFoundException): check_validation_required(tasks, sub_config) - mock_response = Mock() - mock_response.status_code = 500 + mock_response = Mock(status_code=500) get_submission_status_mock.side_effect = HTTPError(response=mock_response) with self.assertRaises(SubmissionStatusException): check_validation_required(tasks, sub_config) diff --git a/tests/test_submit.py b/tests/test_submit.py index 8c697f3..f5df8b3 100644 --- a/tests/test_submit.py +++ b/tests/test_submit.py @@ -41,6 +41,9 @@ def test_submit(self): mock_initiate_response.status_code = 200 mock_initiate_response.json.return_value = {"submissionId": "mock_submission_id", "uploadUrl": "directory to use for upload"} + + + mock_uploaded_response = MagicMock() mock_uploaded_response.status_code = 200 @@ -48,6 +51,7 @@ def test_submit(self): with patch.object(test_submission_ws_client, 'auth') as mocked_auth: with patch.object(self.submitter, 'submission_ws_client', test_submission_ws_client), \ + patch.object(test_submission_ws_client, 'get_submission_status', return_value='OPEN'), \ patch('eva_sub_cli.submission_ws.requests.post', return_value=mock_initiate_response) as mock_post, \ patch('eva_sub_cli.submission_ws.requests.put', return_value=mock_uploaded_response) as mock_put, \ patch.object(StudySubmitter, '_upload_submission'), \ @@ -84,6 +88,7 @@ def test_submit_with_config(self): with patch.object(test_submission_ws_client, 'auth') as mocked_auth: with patch.object(self.submitter, 'submission_ws_client', test_submission_ws_client), \ + patch.object(test_submission_ws_client, 'get_submission_status', return_value='OPEN'), \ patch('eva_sub_cli.submission_ws.requests.post', return_value=mock_initiate_response) as mock_post, \ patch('eva_sub_cli.submission_ws.requests.put', return_value=mock_uploaded_response) as mock_put, \ patch.object(StudySubmitter, '_upload_submission'): @@ -100,6 +105,24 @@ def test_submit_with_config(self): assert sub_config_data[SUB_CLI_CONFIG_KEY_SUBMISSION_ID] == "mock_submission_id" assert sub_config_data[SUB_CLI_CONFIG_KEY_SUBMISSION_UPLOAD_URL] == "directory to use for upload" + def test_submit_when_already_completed(self): + test_submission_ws_client = SubmissionWSClient() + + with patch.object(self.submitter, 'submission_ws_client', test_submission_ws_client), \ + patch.object(test_submission_ws_client, 'get_submission_status', return_value='UPLOADED') as mock_get_submission_status, \ + patch.object(self.submitter, 'submission_dir', self.test_sub_dir), \ + patch.object(self.submitter, '_initiate_submission') as mock_initiate, \ + patch.object(self.submitter, '_upload_submission') as mock_upload, \ + patch.object(self.submitter, '_complete_submission') as mock_complete: + + self.submitter.sub_config.set(READY_FOR_SUBMISSION_TO_EVA, value=True) + self.submitter.sub_config.set(SUB_CLI_CONFIG_KEY_SUBMISSION_UPLOAD_URL, value='https://example.com') + self.submitter.submit() + # No call was made to the end point or the upload + mock_initiate.assert_not_called() + mock_upload.assert_not_called() + mock_complete.assert_not_called() + def test_sub_config_file_creation(self): assert is_submission_dir_writable(self.test_sub_dir) self.submitter.sub_config.set('test_key', value='test_value') From 5baa584032fcee733f58155b4c98bbe963a8b689 Mon Sep 17 00:00:00 2001 From: tcezard Date: Thu, 3 Oct 2024 13:23:38 +0100 Subject: [PATCH 2/4] Fix tests --- tests/test_orchestrator.py | 3 ++- tests/test_submit.py | 7 +++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/test_orchestrator.py b/tests/test_orchestrator.py index c033bc3..a491875 100644 --- a/tests/test_orchestrator.py +++ b/tests/test_orchestrator.py @@ -102,7 +102,8 @@ def test_orchestrate_validate_submit(self): patch('eva_sub_cli.orchestrator.WritableConfig') as m_config, \ patch('eva_sub_cli.orchestrator.get_project_title_and_create_vcf_files_mapping') as m_get_project_title_and_create_vcf_files_mapping, \ patch('eva_sub_cli.orchestrator.DockerValidator') as m_docker_validator, \ - patch('eva_sub_cli.orchestrator.StudySubmitter') as m_submitter: + patch('eva_sub_cli.orchestrator.StudySubmitter') as m_submitter, \ + patch('eva_sub_cli.orchestrator.check_validation_required', return_value=True): # Empty config config = WritableConfig() m_config.return_value = config diff --git a/tests/test_submit.py b/tests/test_submit.py index f5df8b3..15d7c67 100644 --- a/tests/test_submit.py +++ b/tests/test_submit.py @@ -42,8 +42,6 @@ def test_submit(self): mock_initiate_response.json.return_value = {"submissionId": "mock_submission_id", "uploadUrl": "directory to use for upload"} - - mock_uploaded_response = MagicMock() mock_uploaded_response.status_code = 200 @@ -80,6 +78,7 @@ def test_submit_with_config(self): mock_uploaded_response.status_code = 200 assert is_submission_dir_writable(self.test_sub_dir) + assert not os.path.exists(self.config_file) sub_config = WritableConfig(self.config_file, version='version1.0') sub_config.set(READY_FOR_SUBMISSION_TO_EVA, value=True) sub_config.write() @@ -116,9 +115,9 @@ def test_submit_when_already_completed(self): patch.object(self.submitter, '_complete_submission') as mock_complete: self.submitter.sub_config.set(READY_FOR_SUBMISSION_TO_EVA, value=True) - self.submitter.sub_config.set(SUB_CLI_CONFIG_KEY_SUBMISSION_UPLOAD_URL, value='https://example.com') + self.submitter.sub_config.set(SUB_CLI_CONFIG_KEY_SUBMISSION_UPLOAD_URL, value='directory to use for upload') self.submitter.submit() - # No call was made to the end point or the upload + # No call was made to the end point or the upload because the submission status is 'UPLOADED' mock_initiate.assert_not_called() mock_upload.assert_not_called() mock_complete.assert_not_called() From 97bd33f9f80f3821ba01733f2d0e7eb8cb078a3c Mon Sep 17 00:00:00 2001 From: Timothee Cezard Date: Thu, 3 Oct 2024 17:41:56 +0100 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: April Shen --- eva_sub_cli/submit.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eva_sub_cli/submit.py b/eva_sub_cli/submit.py index 15ce14c..45f23fc 100644 --- a/eva_sub_cli/submit.py +++ b/eva_sub_cli/submit.py @@ -98,7 +98,7 @@ def submit(self): if not self.is_submission_status_open(): self.warning(f'You requested the submission using {self.submission_dir}. ' - f'This contains directory an already completed submission. ' + f'This directory contains an already completed submission. ' f'Please create a new submission directory to resubmit.') return From 004210551af05729db47c359fcb25e1321b0c34f Mon Sep 17 00:00:00 2001 From: tcezard Date: Thu, 3 Oct 2024 17:46:22 +0100 Subject: [PATCH 4/4] Address review comments --- tests/test_submit.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/test_submit.py b/tests/test_submit.py index 15d7c67..3c49e19 100644 --- a/tests/test_submit.py +++ b/tests/test_submit.py @@ -52,8 +52,7 @@ def test_submit(self): patch.object(test_submission_ws_client, 'get_submission_status', return_value='OPEN'), \ patch('eva_sub_cli.submission_ws.requests.post', return_value=mock_initiate_response) as mock_post, \ patch('eva_sub_cli.submission_ws.requests.put', return_value=mock_uploaded_response) as mock_put, \ - patch.object(StudySubmitter, '_upload_submission'), \ - patch.object(self.submitter, 'submission_dir', self.test_sub_dir): + patch.object(StudySubmitter, '_upload_submission'): mocked_auth.token = self.token self.submitter.sub_config.set(READY_FOR_SUBMISSION_TO_EVA, value=True) self.submitter.submit() @@ -109,10 +108,10 @@ def test_submit_when_already_completed(self): with patch.object(self.submitter, 'submission_ws_client', test_submission_ws_client), \ patch.object(test_submission_ws_client, 'get_submission_status', return_value='UPLOADED') as mock_get_submission_status, \ - patch.object(self.submitter, 'submission_dir', self.test_sub_dir), \ patch.object(self.submitter, '_initiate_submission') as mock_initiate, \ patch.object(self.submitter, '_upload_submission') as mock_upload, \ - patch.object(self.submitter, '_complete_submission') as mock_complete: + patch.object(self.submitter, '_complete_submission') as mock_complete, \ + patch.object(self.submitter, 'warning') as mock_warning: self.submitter.sub_config.set(READY_FOR_SUBMISSION_TO_EVA, value=True) self.submitter.sub_config.set(SUB_CLI_CONFIG_KEY_SUBMISSION_UPLOAD_URL, value='directory to use for upload') @@ -121,6 +120,9 @@ def test_submit_when_already_completed(self): mock_initiate.assert_not_called() mock_upload.assert_not_called() mock_complete.assert_not_called() + mock_warning.assert_called_once_with( + f'You requested the submission using {self.test_sub_dir}. This directory contains an already completed ' + f'submission. Please create a new submission directory to resubmit.') def test_sub_config_file_creation(self): assert is_submission_dir_writable(self.test_sub_dir)