COMPUTE-1764_no_selector_extra_args (#1512)

* COMPUTE-1764_no_selector_extra_args

(feat) parsers.py, tests - disallowed instanceTypeSelector to be passed in extra-args.

* Update src/python/dxpy/cli/parsers.py

Co-authored-by: Yuxin Shi <[email protected]>

* COMPUTE-1764_no_selector_extra_args

(feat) all - account for the cloned jobs and the runtime specs there

* COMPUTE-1764_no_selector_extra_args

(feat) dx.py - changed error type

* COMPUTE-1764_no_selector_extra_args

(feat) dx.py - relaxed exclusivity to allow instance type in runtime to allow cluster spec.

* COMPUTE-1764_no_selector_extra_args

(feat) dx.py - relaxed exclusivity to allow instance type in runtime to allow cluster spec.

* COMPUTE-1764_no_selector_extra_args

(feat) dx.py - relaxed exclusivity to allow instance type in runtime to allow cluster spec.

---------

Co-authored-by: Yuxin Shi <[email protected]>

Author: aogorodnikov-dna

src/python/dxpy/cli/parsers.py

@@ -222,6 +222,45 @@ def process_extra_args(args):
         except:
             raise DXParserError('Value given for --extra-args could not be parsed as JSON')
 
+        # Validate that instanceTypeSelector is not specified in systemRequirements
+        def check_system_requirements(sys_reqs, context="systemRequirements"):
+            if isinstance(sys_reqs, dict):
+                for entry_point, reqs in sys_reqs.items():
+                    if isinstance(reqs, dict) and 'instanceTypeSelector' in reqs:
+                        raise DXParserError(
+                            'instanceTypeSelector cannot be specified in runtime --extra-args. '
+                            'It can only be defined in the dxapp.json at build time.'
+                        )
+
+        if 'systemRequirements' in args.extra_args:
+            check_system_requirements(args.extra_args['systemRequirements'])
+
+        # Check systemRequirementsByExecutable (double-nested structure: executable -> entrypoint -> requirements)
+        if 'systemRequirementsByExecutable' in args.extra_args:
+            sys_reqs_by_exec = args.extra_args['systemRequirementsByExecutable']
+            if isinstance(sys_reqs_by_exec, dict):
+                for executable_id, entrypoints in sys_reqs_by_exec.items():
+                    if isinstance(entrypoints, dict):
+                        check_system_requirements(entrypoints, f"systemRequirementsByExecutable.{executable_id}")
+
+        # Also check regionalOptions
+        if 'regionalOptions' in args.extra_args:
+            regional_opts = args.extra_args['regionalOptions']
+            if isinstance(regional_opts, dict):
+                for region, region_spec in regional_opts.items():
+                    if isinstance(region_spec, dict):
+                        if 'systemRequirements' in region_spec:
+                            check_system_requirements(region_spec['systemRequirements'],
+                                                     f"regionalOptions.{region}.systemRequirements")
+                        # Also check systemRequirementsByExecutable within regionalOptions
+                        if 'systemRequirementsByExecutable' in region_spec:
+                            sys_reqs_by_exec = region_spec['systemRequirementsByExecutable']
+                            if isinstance(sys_reqs_by_exec, dict):
+                                for executable_id, entrypoints in sys_reqs_by_exec.items():
+                                    if isinstance(entrypoints, dict):
+                                        check_system_requirements(entrypoints,
+                                                                f"regionalOptions.{region}.systemRequirementsByExecutable.{executable_id}")
+
 exec_input_args = argparse.ArgumentParser(add_help=False)
 exec_input_args.add_argument('-i', '--input', help=fill('An input to be added using "<input name>[:<class>]=<input value>" (provide "class" if there is no input spec; it can be any job IO class, e.g. "string", "array:string", or "array"; if "class" is "array" or not specified, the value will be attempted to be parsed as JSON and is otherwise treated as a string)', width_adjustment=-24), action='append')
 exec_input_args.add_argument('-j', '--input-json', help=fill('The full input JSON (keys=input field names, values=input field values)', width_adjustment=-24))

src/python/dxpy/scripts/dx.py

@@ -30,7 +30,7 @@
 import dxpy
 from dxpy.scripts import dx_build_app
 from dxpy import workflow_builder
-from dxpy.exceptions import PermissionDenied, InvalidState, ResourceNotFound
+from dxpy.exceptions import PermissionDenied, InvalidState, ResourceNotFound, DXCLIError
 
 from ..cli import try_call, prompt_for_yn, INTERACTIVE_CLI
 from ..cli import workflow as workflow_cli
@@ -3191,27 +3191,39 @@ def run_body(args, executable, dest_proj, dest_path, preset_inputs=None, input_n
         # however when cloning from an analysis, the temporary workflow already has the cloned spec as its default, so no need to merge 1) and 2) here
         cloned_system_requirements = copy.deepcopy(args.cloned_job_desc).get("systemRequirements", {})
         cloned_instance_type = SystemRequirementsDict.from_sys_requirements(cloned_system_requirements, _type='instanceType')
+        cloned_instance_type_selector = SystemRequirementsDict.from_sys_requirements(cloned_system_requirements, _type='instanceTypeSelector')
         cloned_cluster_spec = SystemRequirementsDict.from_sys_requirements(cloned_system_requirements, _type='clusterSpec')
         cloned_fpga_driver = SystemRequirementsDict.from_sys_requirements(cloned_system_requirements, _type='fpgaDriver')
         cloned_nvidia_driver = SystemRequirementsDict.from_sys_requirements(cloned_system_requirements, _type='nvidiaDriver')
         cloned_system_requirements_by_executable = args.cloned_job_desc.get("mergedSystemRequirementsByExecutable", {}) or {}
     else:
         cloned_system_requirements = {}
-        cloned_instance_type, cloned_cluster_spec, cloned_fpga_driver, cloned_nvidia_driver = (
-            SystemRequirementsDict({}), SystemRequirementsDict({}), SystemRequirementsDict({}), SystemRequirementsDict({}))
+        cloned_instance_type, cloned_instance_type_selector, cloned_cluster_spec, cloned_fpga_driver, cloned_nvidia_driver = (
+            SystemRequirementsDict({}), SystemRequirementsDict({}), SystemRequirementsDict({}), SystemRequirementsDict({}), SystemRequirementsDict({}))
         cloned_system_requirements_by_executable = {}
 
     # convert runtime --instance-type into mapping {entrypoint:{'instanceType':xxx}}
     # here the args.instance_type no longer contains specifications for stage sys reqs
     if args.instance_type:
+        # Runtime --instance-type overrides instanceTypeSelector from the executable's runSpec
+        # When instanceType is specified at runtime, instanceTypeSelector should not be passed to the API
         requested_instance_type = SystemRequirementsDict.from_instance_type(args.instance_type)
         if not isinstance(args.instance_type, basestring):
             requested_instance_type = SystemRequirementsDict(merge(cloned_instance_type.as_dict(), requested_instance_type.as_dict()))
     else:
+        # If no runtime instanceType, use cloned instanceType (not instanceTypeSelector)
+        # instanceTypeSelector from the executable's runSpec will be resolved by the API
         requested_instance_type = cloned_instance_type
 
     # convert runtime --instance-count into mapping {entrypoint:{'clusterSpec':{'initialInstanceCount': N}}})
     if args.instance_count:
+        # Validate that we're not mixing clusterSpec with instanceTypeSelector
+        # clusterSpec and instanceTypeSelector are mutually exclusive at build time
+        # However, if instanceType is provided (either from runtime or cloned job), it overrides instanceTypeSelector
+        # So we only raise an error if instanceTypeSelector exists AND no instanceType override is present
+        if cloned_instance_type_selector.as_dict() and not requested_instance_type.as_dict():
+            raise DXCLIError("Cannot specify --instance-count when cloning a job that uses instanceTypeSelector "
+                            "without providing --instance-type. instanceTypeSelector and clusterSpec are mutually exclusive.")
         # retrieve the full cluster spec defined in executable's runSpec.systemRequirements
         # and overwrite the field initialInstanceCount with the runtime mapping
         requested_instance_count = SystemRequirementsDict.from_instance_count(args.instance_count)        
@@ -3232,8 +3244,11 @@ def run_body(args, executable, dest_proj, dest_path, preset_inputs=None, input_n
     requested_fpga_driver = cloned_fpga_driver
     requested_nvidia_driver = cloned_nvidia_driver
 
+
     # combine the requested instance type, full cluster spec, fpga spec, nvidia spec
     # into the runtime systemRequirements
+    # Note: instanceTypeSelector is NOT included here as it's build-time only
+    # If runtime instanceType was provided, it overrides instanceTypeSelector
     requested_system_requirements = (requested_instance_type + requested_cluster_spec + requested_fpga_driver +
                                      requested_nvidia_driver).as_dict()
 

src/python/dxpy/system_requirements.py

@@ -83,9 +83,9 @@ def from_sys_requirements(cls, system_requirements, _type='all'):
         """
         Returns SystemRequirementsDict encapsulating system requirements.
         It can extract only entrypoints with specific fields ('clusterSpec',
-        'instanceType', etc), depending on the value of _type.
+        'instanceType', 'instanceTypeSelector', etc), depending on the value of _type.
         """
-        allowed_types = ['all', 'clusterSpec', 'instanceType', 'fpgaDriver', 'nvidiaDriver']
+        allowed_types = ['all', 'clusterSpec', 'instanceType', 'instanceTypeSelector', 'fpgaDriver', 'nvidiaDriver']
         if _type not in (allowed_types):
             raise DXError("Expected '_type' to be one of the following: {}".format(allowed_types))
 

src/python/test/test_dxclient.py

@@ -3280,6 +3280,46 @@ def test_dx_run_extra_args(self):
         with self.assertSubprocessFailure(stderr_regexp='JSON', exit_code=3):
             run("dx run " + applet_id + " --extra-args not-a-JSON-string")
 
+    def test_dx_run_extra_args_instanceTypeSelector_rejected(self):
+        # Validate that instanceTypeSelector cannot be specified in runtime extra-args
+        applet_id = dxpy.api.applet_new({"project": self.project,
+                                         "dxapi": "1.0.0",
+                                         "runSpec": {"interpreter": "bash",
+                                                     "distribution": "Ubuntu",
+                                                     "release": "14.04",
+                                                     "code": "echo 'hello'"}
+                                         })['id']
+
+        # Test with wildcard entry point
+        with self.assertSubprocessFailure(stderr_regexp='instanceTypeSelector cannot be specified in runtime', exit_code=3):
+            run('dx run ' + applet_id + ' --extra-args ' +
+                '\'{"systemRequirements": {"*": {"instanceTypeSelector": {"allowedInstanceTypes": ["mem2_ssd1_v2_x2"]}}}}\'')
+
+        # Test with specific entry point
+        with self.assertSubprocessFailure(stderr_regexp='instanceTypeSelector cannot be specified in runtime', exit_code=3):
+            run('dx run ' + applet_id + ' --extra-args ' +
+                '\'{"systemRequirements": {"main": {"instanceTypeSelector": {"allowedInstanceTypes": ["mem2_ssd1_v2_x2"]}}}}\'')
+
+        # Test with regionalOptions
+        with self.assertSubprocessFailure(stderr_regexp='instanceTypeSelector cannot be specified in runtime', exit_code=3):
+            run('dx run ' + applet_id + ' --extra-args ' +
+                '\'{"regionalOptions": {"aws:us-east-1": {"systemRequirements": {"*": {"instanceTypeSelector": {"allowedInstanceTypes": ["mem2_ssd1_v2_x2"]}}}}}}\'')
+
+        # Test with systemRequirementsByExecutable
+        with self.assertSubprocessFailure(stderr_regexp='instanceTypeSelector cannot be specified in runtime', exit_code=3):
+            run('dx run ' + applet_id + ' --extra-args ' +
+                '\'{"systemRequirementsByExecutable": {"' + applet_id + '": {"main": {"instanceTypeSelector": {"allowedInstanceTypes": ["mem2_ssd1_v2_x2"]}}}}}\'')
+
+        # Test with systemRequirementsByExecutable with wildcard entry point
+        with self.assertSubprocessFailure(stderr_regexp='instanceTypeSelector cannot be specified in runtime', exit_code=3):
+            run('dx run ' + applet_id + ' --extra-args ' +
+                '\'{"systemRequirementsByExecutable": {"' + applet_id + '": {"*": {"instanceTypeSelector": {"allowedInstanceTypes": ["mem2_ssd1_v2_x2"]}}}}}\'')
+
+        # Test with regionalOptions.systemRequirementsByExecutable
+        with self.assertSubprocessFailure(stderr_regexp='instanceTypeSelector cannot be specified in runtime', exit_code=3):
+            run('dx run ' + applet_id + ' --extra-args ' +
+                '\'{"regionalOptions": {"aws:us-east-1": {"systemRequirementsByExecutable": {"' + applet_id + '": {"main": {"instanceTypeSelector": {"allowedInstanceTypes": ["mem2_ssd1_v2_x2"]}}}}}}}\'')
+
     def test_dx_run_sys_reqs(self):
         app_spec = {"project": self.project,
                     "dxapi": "1.0.0",
@@ -3378,6 +3418,73 @@ def test_dx_run_clone_nvidia_driver(self):
         cloned_job_desc = dxpy.api.job_describe(cloned_job_id_nvidia_override)
         assert cloned_job_desc["systemRequirements"]["*"]["nvidiaDriver"] == build_nvidia_version
 
+    def test_dx_run_clone_with_instance_type_selector(self):
+        """
+        Test that when cloning a job from an applet that uses instanceTypeSelector:
+        1. Cloning without runtime args works - instanceTypeSelector is preserved
+        2. Cloning with --instance-type works - runtime instanceType overrides instanceTypeSelector
+        3. Cloning with --instance-count fails - clusterSpec and instanceTypeSelector are mutually exclusive
+        4. Job description contains instanceTypeSelector in systemRequirements
+        """
+        # Create an applet with instanceTypeSelector
+        applet_id = dxpy.api.applet_new({"project": self.project,
+                                         "dxapi": "1.0.0",
+                                         "runSpec": {"interpreter": "bash",
+                                                     "distribution": "Ubuntu",
+                                                     "release": "20.04",
+                                                     "version": "0",
+                                                     "code": "echo 'hello'",
+                                                     "systemRequirements": {
+                                                         "*": {
+                                                             "instanceTypeSelector": {
+                                                                 "allowedInstanceTypes": ["mem1_ssd1_x2", "mem2_ssd1_x2"]
+                                                             }
+                                                         }
+                                                     }}
+                                         })['id']
+
+        # Run the applet to create the origin job
+        # The API will resolve instanceTypeSelector to an actual instanceType
+        origin_job_id = run(f"dx run {applet_id} --brief -y").strip().split('\n')[-1]
+
+        # Verify the origin job description contains instanceTypeSelector
+        origin_job_desc = dxpy.api.job_describe(origin_job_id)
+        assert "systemRequirements" in origin_job_desc
+        assert "*" in origin_job_desc["systemRequirements"]
+        assert "instanceTypeSelector" in origin_job_desc["systemRequirements"]["*"]
+        assert "allowedInstanceTypes" in origin_job_desc["systemRequirements"]["*"]["instanceTypeSelector"]
+
+        # Clone without runtime args - should work, instanceTypeSelector is preserved
+        cloned_job_id_1 = run(f"dx run --clone {origin_job_id} --brief -y").strip()
+        assert cloned_job_id_1.startswith("job-")
+        cloned_job_desc_1 = dxpy.api.job_describe(cloned_job_id_1)
+        # Verify instanceTypeSelector is in the cloned job's systemRequirements
+        assert "instanceTypeSelector" in cloned_job_desc_1["systemRequirements"]["*"]
+
+        # Clone with --instance-type - should work, runtime instanceType overrides instanceTypeSelector
+        cloned_job_id_2 = run(f"dx run --clone {origin_job_id} --instance-type mem2_hdd2_x4 --brief -y").strip()
+        assert cloned_job_id_2.startswith("job-")
+        cloned_job_desc_2 = dxpy.api.job_describe(cloned_job_id_2)
+        # Verify instanceType is used instead of instanceTypeSelector
+        assert "instanceType" in cloned_job_desc_2["systemRequirements"]["*"]
+        assert cloned_job_desc_2["systemRequirements"]["*"]["instanceType"] == "mem2_hdd2_x4"
+        # instanceTypeSelector should not be in the runtime systemRequirements when overridden
+        # TODO uncomment later - for now this is not included in the backend behaviour
+        # assert "instanceTypeSelector" not in cloned_job_desc_2["systemRequirements"]["*"]
+
+        # Clone with --instance-count alone - should fail (clusterSpec and instanceTypeSelector are mutually exclusive)
+        with self.assertSubprocessFailure(stderr_regexp='Cannot specify --instance-count.*instanceTypeSelector.*without providing --instance-type', exit_code=3):
+            run(f"dx run --clone {origin_job_id} --instance-count 3 --brief -y")
+
+        # Clone with both --instance-count and --instance-type - should work (instanceType overrides instanceTypeSelector)
+        cloned_job_id_3 = run(f"dx run --clone {origin_job_id} --instance-count 3 --instance-type mem2_hdd2_x4 --brief -y").strip()
+        assert cloned_job_id_3.startswith("job-")
+        cloned_job_desc_3 = dxpy.api.job_describe(cloned_job_id_3)
+        # Verify instanceType override is applied
+        assert "instanceType" in cloned_job_desc_3["systemRequirements"]["*"]
+        assert cloned_job_desc_3["systemRequirements"]["*"]["instanceType"] == "mem2_hdd2_x4"
+        # Note: When the applet's runSpec doesn't have a clusterSpec defined (only instanceTypeSelector)
+
     def test_dx_run_clone(self):
         applet_id = dxpy.api.applet_new({"project": self.project,
                                          "dxapi": "1.0.0",