From 65751c78cd0e2d2a26b1ff9b04efe832630eb9e2 Mon Sep 17 00:00:00 2001 From: Sean Mackesey Date: Wed, 11 Dec 2024 20:11:41 -0500 Subject: [PATCH] [components] Error on passing both --json-params and EXTRA_ARGS to `dg generate component` (#26418) ## Summary & Motivation Have `dagster-dg` throw an error when both `--json-params` and `EXTRA_ARGS` are passed to `dg generate component`. These are mutually exclusive ways of specifying parameters. ## How I Tested These Changes New unit tests. --- .../dagster-dg/dagster_dg/cli/generate.py | 12 ++++ .../dagster-dg/dagster_dg/generate.py | 2 +- .../cli_tests/test_generate_commands.py | 72 +++++++++++++++++-- python_modules/libraries/dagster-dg/setup.py | 2 +- 4 files changed, 80 insertions(+), 8 deletions(-) diff --git a/python_modules/libraries/dagster-dg/dagster_dg/cli/generate.py b/python_modules/libraries/dagster-dg/dagster_dg/cli/generate.py index a8c18bd490b86..be58f8b0284ea 100644 --- a/python_modules/libraries/dagster-dg/dagster_dg/cli/generate.py +++ b/python_modules/libraries/dagster-dg/dagster_dg/cli/generate.py @@ -153,6 +153,8 @@ def generate_component_command( When key-value pairs are used, the value type will be inferred from the underlying component generation schema. + + It is an error to pass both --json-params and EXTRA_ARGS. """ if not is_inside_code_location_project(Path.cwd()): click.echo( @@ -174,6 +176,16 @@ def generate_component_command( ) sys.exit(1) + if json_params is not None and extra_args: + click.echo( + click.style( + "Detected both --json-params and EXTRA_ARGS. These are mutually exclusive means of passing" + " component generation parameters. Use only one.", + fg="red", + ) + ) + sys.exit(1) + generate_component_instance( Path(context.component_instances_root_path), component_name, diff --git a/python_modules/libraries/dagster-dg/dagster_dg/generate.py b/python_modules/libraries/dagster-dg/dagster_dg/generate.py index f469b9c824e94..6bf0ee7f42b4a 100644 --- a/python_modules/libraries/dagster-dg/dagster_dg/generate.py +++ b/python_modules/libraries/dagster-dg/dagster_dg/generate.py @@ -103,7 +103,7 @@ def generate_component_instance( "component", component_type, name, - *([f"--json-params={json_params}"] if json_params else []), + *(["--json-params", json_params] if json_params else []), *(["--", *extra_args] if extra_args else []), ) execute_code_location_command(Path(component_instance_root_path), code_location_command) diff --git a/python_modules/libraries/dagster-dg/dagster_dg_tests/cli_tests/test_generate_commands.py b/python_modules/libraries/dagster-dg/dagster_dg_tests/cli_tests/test_generate_commands.py index 8275d839690cd..2d99002b9da92 100644 --- a/python_modules/libraries/dagster-dg/dagster_dg_tests/cli_tests/test_generate_commands.py +++ b/python_modules/libraries/dagster-dg/dagster_dg_tests/cli_tests/test_generate_commands.py @@ -33,10 +33,10 @@ def _assert_module_imports(module_name: str): # This is a holder for code that is intended to be written to a file def _example_component_type_baz(): - from typing import Any - + import click from dagster import AssetExecutionContext, Definitions, PipesSubprocessClient, asset from dagster_components import Component, ComponentLoadContext, component + from pydantic import BaseModel _SAMPLE_PIPES_SCRIPT = """ from dagster_pipes import open_dagster_pipes @@ -45,11 +45,22 @@ def _example_component_type_baz(): context.report_asset_materialization({"alpha": "beta"}) """ + class BazGenerateParams(BaseModel): + filename: str = "sample.py" + + @staticmethod + @click.command + @click.option("--filename", type=str, default="sample.py") + def cli(filename: str) -> "BazGenerateParams": + return BazGenerateParams(filename=filename) + @component(name="baz") class Baz(Component): + generate_params_schema = BazGenerateParams + @classmethod - def generate_files(cls, params: Any): - with open("sample.py", "w") as f: + def generate_files(cls, params: BazGenerateParams): + with open(params.filename, "w") as f: f.write(_SAMPLE_PIPES_SCRIPT) def build_defs(self, context: ComponentLoadContext) -> Definitions: @@ -250,18 +261,67 @@ def test_generate_component_type_already_exists_fails(in_deployment: bool) -> No @pytest.mark.parametrize("in_deployment", [True, False]) -def test_generate_component_success(in_deployment: bool) -> None: +def test_generate_component_no_params_success(in_deployment: bool) -> None: runner = CliRunner() with isolated_example_code_location_bar_with_component_type_baz(runner, in_deployment): result = runner.invoke(generate_component_command, ["bar.baz", "qux"]) assert result.exit_code == 0 assert Path("bar/components/qux").exists() - assert Path("bar/components/qux/sample.py").exists() + assert Path("bar/components/qux/sample.py").exists() # default filename + component_yaml_path = Path("bar/components/qux/component.yaml") + assert component_yaml_path.exists() + assert "type: bar.baz" in component_yaml_path.read_text() + + +@pytest.mark.parametrize("in_deployment", [True, False]) +def test_generate_component_json_params_success(in_deployment: bool) -> None: + runner = CliRunner() + with isolated_example_code_location_bar_with_component_type_baz(runner, in_deployment): + result = runner.invoke( + generate_component_command, + ["bar.baz", "qux", "--json-params", '{"filename": "hello.py"}'], + ) + assert result.exit_code == 0 + assert Path("bar/components/qux").exists() + assert Path("bar/components/qux/hello.py").exists() + component_yaml_path = Path("bar/components/qux/component.yaml") + assert component_yaml_path.exists() + assert "type: bar.baz" in component_yaml_path.read_text() + + +@pytest.mark.parametrize("in_deployment", [True, False]) +def test_generate_component_extra_args_success(in_deployment: bool) -> None: + runner = CliRunner() + with isolated_example_code_location_bar_with_component_type_baz(runner, in_deployment): + result = runner.invoke( + generate_component_command, ["bar.baz", "qux", "--", "--filename=hello.py"] + ) + assert result.exit_code == 0 + assert Path("bar/components/qux").exists() + assert Path("bar/components/qux/hello.py").exists() component_yaml_path = Path("bar/components/qux/component.yaml") assert component_yaml_path.exists() assert "type: bar.baz" in component_yaml_path.read_text() +def test_generate_component_json_params_and_extra_args_fails() -> None: + runner = CliRunner() + with isolated_example_code_location_bar_with_component_type_baz(runner): + result = runner.invoke( + generate_component_command, + [ + "bar.baz", + "qux", + "--json-params", + '{"filename": "hello.py"}', + "--", + "--filename=hello.py", + ], + ) + assert result.exit_code != 0 + assert "Detected both --json-params and EXTRA_ARGS" in result.output + + def test_generate_component_outside_code_location_fails() -> None: runner = CliRunner() with isolated_example_deployment_foo(runner): diff --git a/python_modules/libraries/dagster-dg/setup.py b/python_modules/libraries/dagster-dg/setup.py index 767da03708ef4..78f487255a4cc 100644 --- a/python_modules/libraries/dagster-dg/setup.py +++ b/python_modules/libraries/dagster-dg/setup.py @@ -46,6 +46,6 @@ def get_version() -> str: ] }, extras_require={ - "test": ["pytest"], + "test": ["click", "pydantic", "pytest"], }, )