From 921be70255c72f9a20e1f7d124d6ea2a10fac88d Mon Sep 17 00:00:00 2001 From: Juliana Karoline de Sousa Date: Thu, 19 Oct 2023 17:20:14 -0300 Subject: [PATCH] [ASP-3985] Add extra params create-job-script (#387) * Create question helper * Add cluster_name, exec_dir and download to render * Update logic for default execution_dir * Add unit tests * Lint code * Requested changes in code review * Update logic for download * Add entry to CHANGELOG --- jobbergate-cli/CHANGELOG.md | 9 +- .../subapps/applications/questions.py | 2 +- .../jobbergate_cli/subapps/job_scripts/app.py | 88 +++++++++++++++++-- .../subapps/job_scripts/tools.py | 25 +++++- .../subapps/job_submissions/tools.py | 32 ++++--- .../tests/subapps/job_scripts/test_app.py | 76 +++++++++++++++- .../tests/subapps/job_scripts/test_tools.py | 63 +++++++++++++ .../subapps/job_submissions/test_tools.py | 33 ++++++- 8 files changed, 296 insertions(+), 32 deletions(-) diff --git a/jobbergate-cli/CHANGELOG.md b/jobbergate-cli/CHANGELOG.md index 88dadaeb..09e10669 100644 --- a/jobbergate-cli/CHANGELOG.md +++ b/jobbergate-cli/CHANGELOG.md @@ -3,16 +3,17 @@ This file keeps track of all notable changes to jobbergate-cli ## Unreleased +- Added --cluster-name, --execution-directory and --download parameters to create-job-script command on submit mode ## 4.1.0a2 -- 2023-10-10 - Added a `create` for Job Scripts to to create without Template. (former `create` renamed to `render`) - Fixed help information for id on `get-job-script`, `download-job-script`, and `get-job-submission` commands - Added short arguments for backward compatiblity -- Ignore username and password arguments if provided, aiming to keep backward compatibility -- Add support to select applications by identifier in update and delete commands -- Add show-files command to compat mode -- Add support to select all and deselect all options in checkboxes using Ctrl+A and Ctrl+R as shortcuts +- Ignored username and password arguments if provided, aiming to keep backward compatibility +- Added support to select applications by identifier in update and delete commands +- Added show-files command to compat mode +- Added support to select all and deselect all options in checkboxes using Ctrl+A and Ctrl+R as shortcuts ## 4.1.0a1 -- 2023-10-02 diff --git a/jobbergate-cli/jobbergate_cli/subapps/applications/questions.py b/jobbergate-cli/jobbergate_cli/subapps/applications/questions.py index 83ae621f..039e45d4 100644 --- a/jobbergate-cli/jobbergate_cli/subapps/applications/questions.py +++ b/jobbergate-cli/jobbergate_cli/subapps/applications/questions.py @@ -201,7 +201,7 @@ def __init__(self, variablename: str, message: str, choices: list, **kwargs): variablename, message + " (press CTRL+A to SELECT ALL or CTRL+R to DESELECT ALL)", inquirer_type=inquirer.Checkbox, - **kwargs + **kwargs, ) self.inquirer_kwargs.update(choices=choices) diff --git a/jobbergate-cli/jobbergate_cli/subapps/job_scripts/app.py b/jobbergate-cli/jobbergate_cli/subapps/job_scripts/app.py index 36449dd1..6c8bdde1 100644 --- a/jobbergate-cli/jobbergate_cli/subapps/job_scripts/app.py +++ b/jobbergate-cli/jobbergate_cli/subapps/job_scripts/app.py @@ -8,6 +8,7 @@ import typer +from jobbergate_cli.config import settings from jobbergate_cli.constants import SortOrder from jobbergate_cli.exceptions import Abort, handle_abort from jobbergate_cli.render import StyleMapper, render_list_results, render_single_result, terminal_message @@ -16,6 +17,7 @@ from jobbergate_cli.subapps.job_scripts.tools import ( download_job_script_files, fetch_job_script_data, + question_helper, render_job_script, save_job_script_files, upload_job_script_files, @@ -210,6 +212,25 @@ def render( """ ), ), + cluster_name: Optional[str] = typer.Option( + None, + help="The name of the cluster where the job should be submitted (i.g. 'nash-staging')", + ), + execution_directory: Optional[pathlib.Path] = typer.Option( + None, + help=dedent( + """ + The path on the cluster where the job script should be executed. + If provided as a relative path, it will be converted as an absolute path from your current + working directory. If you use "~" to denote your home directory, the path will be expanded to an + absolute path for your home directory on *this* machine. + """ + ).strip(), + ), + download: Optional[bool] = typer.Option( + None, + help="Download the job script files to the current working directory", + ), fast: bool = typer.Option( False, "--fast", @@ -244,21 +265,70 @@ def render( title="Created Job Script", ) - # `submit` will be `None` --submit/--no-submit flag was not set - if submit is None: - # If not running in "fast" mode, ask the user what to do. - if not fast: - submit = typer.confirm("Would you like to submit this job immediately?") + # `submit` will be `None` when --submit/--no-submit flag was not set + if submit is None and (cluster_name or execution_directory): + # If the user specified --cluster-name or --execution-directory, they must also specify --submit + raise Abort( + "You must specify --cluster-name and --execution-directory only on submit mode.", + subject="Incorrect parameters", + support=True, + ) + + """ + Invoke `question_helper` to handle the logic for parameters that are not specified by the user. + + If the user specified --fast, the `question_helper` will not invoke the function and will + instead return the default answer. If the user did not specify --fast, the `question_helper` + will invoke the function and return the user's answer. + """ + + submit = question_helper( + question_func=typer.confirm, + text="Would you like to submit this job immediately?", + default=True, + fast=fast, + actual_value=submit, + ) + + download = question_helper( + question_func=typer.confirm, + text="Would you like to download the job script files?", + default=True, + fast=fast, + actual_value=download, + ) - # Otherwise, assume that the job script should be submitted immediately - else: - submit = True + if download: + download_job_script_files(job_script_result.id, jg_ctx) if not submit: return + cluster_name = question_helper( + question_func=typer.prompt, + text="What cluster should this job be submitted to?", + default=settings.DEFAULT_CLUSTER_NAME, + fast=fast, + actual_value=cluster_name, + ) + execution_directory = question_helper( + question_func=typer.prompt, + text="Where should this job be executed?", + default=pathlib.Path.cwd(), + fast=fast, + actual_value=execution_directory, + ) + try: - job_submission_result = create_job_submission(jg_ctx, job_script_result.id, job_script_result.name) + job_submission_result = create_job_submission( + jg_ctx, + job_script_result.id, + job_script_result.name, + job_script_result.description, + cluster_name, + execution_directory, + param_file, + ) except Exception as err: raise Abort( "Failed to immediately submit the job after job script creation.", diff --git a/jobbergate-cli/jobbergate_cli/subapps/job_scripts/tools.py b/jobbergate-cli/jobbergate_cli/subapps/job_scripts/tools.py index 587aaf76..c1d0d8c2 100644 --- a/jobbergate-cli/jobbergate_cli/subapps/job_scripts/tools.py +++ b/jobbergate-cli/jobbergate_cli/subapps/job_scripts/tools.py @@ -6,7 +6,7 @@ import pathlib import re import tempfile -from typing import Any, Dict, List, Optional, cast +from typing import Any, Callable, Dict, List, Optional, cast from loguru import logger @@ -386,3 +386,26 @@ def download_job_script_files(id: int, jg_ctx: JobbergateContext) -> List[pathli destination_path=pathlib.Path.cwd(), ) return downloaded_files + + +def question_helper(question_func: Callable, text: str, default: Any, fast: bool, actual_value: Optional[Any]): + """ + Helper function for asking questions to the user. + + :param Callable question_func: The function to use to ask the question + :param str text: The text of the question to ask + :param Any default: The default value to use if the user does not provide one + :param bool fast: Whether to use default answers (when available) instead of asking the user + :param Any actual_value: The actual value provided by the user, if any + + :returns: `actual_value` or `default` or the value provided by the user + + The `actual_value` has the most priority and will be returned if it is not None. + After evaluating the `actual_value`, the fast mode will determine if the default value will be used. + Otherwise, the question will be prompted to the user. + """ + if actual_value is not None: + return actual_value + if fast: + return default + return question_func(text, default=default) diff --git a/jobbergate-cli/jobbergate_cli/subapps/job_submissions/tools.py b/jobbergate-cli/jobbergate_cli/subapps/job_submissions/tools.py index 4f24cfdb..8af3fe94 100644 --- a/jobbergate-cli/jobbergate_cli/subapps/job_submissions/tools.py +++ b/jobbergate-cli/jobbergate_cli/subapps/job_submissions/tools.py @@ -24,16 +24,19 @@ def create_job_submission( """ Create a Job Submission from the given Job Script. - :param: jg_ctx: The JobbergateContext. Used to retrieve the client for requests - and the email of the submitting user - :param: job_script_id: The ``id`` of the Job Script to submit to Slurm - :param: name: The name to attach to the Job Submission - :param: description: An optional description that may be added to the Job Submission - :param: cluster_name: An optional cluster_name for the cluster where the job should be executed, - If left off, it will default to the DEFAULT_CLUSTER_NAME from the settings. - If no default is set, an exception will be raised. - :param: execution_directory: An optional directory where the job should be executed. If provided as a relative path, - it will be constructed as an absolute path relative to the current working directory. + :param: jg_ctx: The JobbergateContext. Used to retrieve the client for requests + and the email of the submitting user + :param: job_script_id: The ``id`` of the Job Script to submit to Slurm + :param: name: The name to attach to the Job Submission + :param: description: An optional description that may be added to the Job Submission + :param: cluster_name: An optional cluster_name for the cluster where the job should be executed, + If left off, it will default to the DEFAULT_CLUSTER_NAME from the settings. + If no default is set, an exception will be raised. + :param: execution_directory: An optional directory where the job should be executed. If provided as a + relative path, it will be constructed as an absolute path relative to + the current working directory. + :param: execution_parameters_file: An optional file containing the execution parameters for the job. + :returns: The Job Submission data returned by the API after creating the new Job Submission """ @@ -60,11 +63,12 @@ def create_job_submission( cluster_name=cluster_name, ) - if execution_directory is not None: + if execution_directory is None: + execution_directory = Path.cwd() + else: if not execution_directory.is_absolute(): - execution_directory = Path.cwd() / execution_directory - execution_directory.resolve() - job_submission_data.execution_directory = execution_directory + execution_directory = (Path.cwd() / execution_directory).resolve() + job_submission_data.execution_directory = execution_directory if execution_parameters_file is not None: job_submission_data.execution_parameters = validate_parameter_file(execution_parameters_file) diff --git a/jobbergate-cli/tests/subapps/job_scripts/test_app.py b/jobbergate-cli/tests/subapps/job_scripts/test_app.py index d9364c60..4abe5a94 100644 --- a/jobbergate-cli/tests/subapps/job_scripts/test_app.py +++ b/jobbergate-cli/tests/subapps/job_scripts/test_app.py @@ -6,6 +6,7 @@ import httpx import pytest +from jobbergate_cli.config import settings from jobbergate_cli.schemas import ( ApplicationResponse, JobScriptFiles, @@ -291,7 +292,8 @@ def test_render__non_fast_mode_and_job_submission( """ ) ), - input="y\n", # To confirm that the job should be submitted right away + # To confirm that the job should be submitted to the default cluster, in the current dir and not downloaded + input=f"y\nn\n{settings.DEFAULT_CLUSTER_NAME}\n.\n", ) assert result.exit_code == 0, f"render failed: {result.stdout}" assert mocked_fetch_application_data.called_once_with( @@ -411,6 +413,7 @@ def test_render__with_fast_mode_and_no_job_submission( --param-file={param_file_path} --fast --no-submit + --no-download {sbatch_params} """ ) @@ -453,6 +456,77 @@ def test_render__with_fast_mode_and_no_job_submission( ) +def test_render__submit_is_none_and_cluster_name_is_defined( + respx_mock, + make_test_app, + dummy_context, + dummy_module_source, + dummy_application_data, + dummy_job_script_data, + dummy_domain, + cli_runner, + tmp_path, + attach_persona, + mocker, +): + application_response = ApplicationResponse(**dummy_application_data[0]) + + job_script_data = dummy_job_script_data[0] + + render_route = respx_mock.post( + f"{dummy_domain}/jobbergate/job-scripts/render-from-template/{application_response.id}" + ) + render_route.mock( + return_value=httpx.Response( + httpx.codes.CREATED, + json=job_script_data, + ), + ) + + sbatch_params = " ".join(f"--sbatch-params={i}" for i in (1, 2, 3)) + + param_file_path = tmp_path / "param_file.json" + param_file_path.write_text( + json.dumps( + dict( + foo="oof", + bar="rab", + baz="zab", + ) + ) + ) + + attach_persona("dummy@dummy.com") + + test_app = make_test_app("render", render) + mocked_render = mocker.patch("jobbergate_cli.subapps.job_scripts.app.render_single_result") + mocked_abort = mocker.patch("jobbergate_cli.subapps.job_scripts.app.Abort") + + cli_runner.invoke( + test_app, + shlex.split( + unwrap( + f""" + render --name=dummy-name + --application-id={application_response.id} + --param-file={param_file_path} + --cluster-name=dummy-cluster + {sbatch_params} + """ + ) + ), + ) + assert mocked_render.called_once_with( + dummy_context, + JobScriptResponse(**job_script_data), + title="Created Job Script", + hidden_fields=HIDDEN_FIELDS, + ) + assert mocked_abort.called_once_with( + "You must specify --cluster-name and --execution-directory only on submit mode.", + ) + + @pytest.mark.parametrize("id_flag,separator", [("--id", "="), ("-i", " ")]) def test_update__makes_request_and_renders_results( respx_mock, diff --git a/jobbergate-cli/tests/subapps/job_scripts/test_tools.py b/jobbergate-cli/tests/subapps/job_scripts/test_tools.py index 9b989a56..77cc5a9d 100644 --- a/jobbergate-cli/tests/subapps/job_scripts/test_tools.py +++ b/jobbergate-cli/tests/subapps/job_scripts/test_tools.py @@ -1,6 +1,7 @@ import importlib import json import pathlib +from unittest import mock import httpx import pytest @@ -13,6 +14,7 @@ fetch_job_script_data, flatten_param_dict, get_template_output_name_mapping, + question_helper, remove_prefix_suffix, render_job_script, save_job_script_files, @@ -201,6 +203,67 @@ def test_render_job_script__without_a_name( assert actual_job_script_data == JobScriptResponse.parse_obj(desired_job_script_data) +def test_question_helper__return_actual_value_when_actual_value_is_not_none(): + question_func = mock.Mock() + + assert ( + question_helper( + question_func=question_func, + text="Give me foo", + default="foo", + fast=True, + actual_value="bar", + ) + == "bar" + ) + + assert question_func.not_called() + + assert ( + question_helper( + question_func=question_func, + text="Give me foo", + default="foo", + fast=False, + actual_value="bar", + ) + == "bar" + ) + + assert question_func.not_called() + + +def test_question_helper__return_default_when_actual_value_is_none_on_fast_mode(): + question_func = mock.Mock() + + assert ( + question_helper( + question_func=question_func, + text="Give me foo", + default="foo", + fast=True, + actual_value=None, + ) + == "foo" + ) + + assert question_func.not_called() + + +def test_question_helper__ask_question_when_actual_value_is_none_on_non_fast_mode(): + question_func = mock.Mock() + + question_helper( + question_func=question_func, + text="Give me foo", + default="foo", + fast=False, + actual_value=None, + ) + + assert question_func.called_once_with("Give me foo", default="foo") + + class TestSaveJobScriptFiles: """ Test the save_job_script_files function. diff --git a/jobbergate-cli/tests/subapps/job_submissions/test_tools.py b/jobbergate-cli/tests/subapps/job_submissions/test_tools.py index 9a45f807..85ad01c8 100644 --- a/jobbergate-cli/tests/subapps/job_submissions/test_tools.py +++ b/jobbergate-cli/tests/subapps/job_submissions/test_tools.py @@ -151,7 +151,7 @@ def test_create_job_submission__throws_exception_with_no_explicit_or_default_clu ) -def test_create_job_submission__with_execution_dir( +def test_create_job_submission__with_explicit_execution_dir( respx_mock, dummy_job_submission_data, dummy_domain, @@ -198,6 +198,35 @@ def test_create_job_submission__with_execution_dir( payload = json.loads(create_job_submission_route.calls.last.request.content) assert payload["execution_directory"] == str(pathlib.Path.cwd() / "./some/relative/path") + +def test_create_job_submission__with_default_execution_dir( + mocker, + respx_mock, + dummy_job_submission_data, + dummy_domain, + dummy_context, + attach_persona, + seed_clusters, +): + job_submission_data = dummy_job_submission_data[0] + job_submission_name = job_submission_data["name"] + job_submission_description = job_submission_data["description"] + + job_script_id = job_submission_data["job_script_id"] + + create_job_submission_route = respx_mock.post(f"{dummy_domain}/jobbergate/job-submissions") + create_job_submission_route.mock( + return_value=httpx.Response( + httpx.codes.CREATED, + json=job_submission_data, + ), + ) + + attach_persona("dummy@dummy.com") + seed_clusters("dummy-cluster") + + mocker.patch("jobbergate_cli.subapps.job_submissions.tools.Path.cwd", return_value=pathlib.Path("/some/fake/path")) + create_job_submission( dummy_context, job_script_id, @@ -207,7 +236,7 @@ def test_create_job_submission__with_execution_dir( execution_directory=None, ) payload = json.loads(create_job_submission_route.calls.last.request.content) - assert payload["execution_directory"] is None + assert payload["execution_directory"] == "/some/fake/path" def test_fetch_job_submission_data__success__using_id(