Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dg] Move check impl to lighter weight JSON validation #27214

Merged
merged 10 commits into from
Jan 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import click
from dagster.version import __version__

from dagster_components.cli.check import check_cli
from dagster_components.cli.list import list_cli
from dagster_components.cli.scaffold import scaffold_cli
from dagster_components.core.component import BUILTIN_MAIN_COMPONENT_ENTRY_POINT
Expand All @@ -12,7 +11,6 @@ def create_dagster_components_cli():
commands = {
"scaffold": scaffold_cli,
"list": list_cli,
"check": check_cli,
}

@click.group(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,8 @@ def _add_component_type_to_output(


@list_cli.command(name="local-component-types")
@click.pass_context
@click.argument("component_directories", nargs=-1, type=click.Path(exists=True))
def list_local_component_types_command(
ctx: click.Context, component_directories: Sequence[str]
) -> None:
def list_local_component_types_command(component_directories: Sequence[str]) -> None:
"""List local Dagster components found in the specified directories."""
output: list = []
for component_directory in component_directories:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@
"""

from dagster._core.definitions.definitions_class import Definitions
from typing_extensions import Self

from dagster_components import Component, component_type
from dagster_components.core.component import ComponentLoadContext
from dagster_components.core.schema.base import ComponentSchemaBaseModel
from typing_extensions import Self


class MyComponentSchema(ComponentSchemaBaseModel):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ class ComponentValidationTestCase:
"""

component_path: str
component_type_filepath: Path
component_type_filepath: Optional[Path]
should_error: bool
validate_error_msg: Optional[Callable[[str], None]] = None
validate_error_msg_additional_cli: Optional[Callable[[str], None]] = None
check_error_msg: Optional[Callable[[str], None]] = None


def msg_includes_all_of(*substrings: str) -> Callable[[str], None]:
Expand All @@ -31,15 +31,22 @@ def _validate_error_msg(msg: str) -> None:
validate_error_msg=msg_includes_all_of(
"component.yaml:5", "params.an_int", "Input should be a valid integer"
),
check_error_msg=msg_includes_all_of(
"component.yaml:5",
"params.an_int",
"{} is not of type 'integer'",
),
)

BASIC_MISSING_VALUE = ComponentValidationTestCase(
component_path="validation/basic_component_missing_value",
component_type_filepath=Path(__file__).parent / "basic_components.py",
should_error=True,
validate_error_msg=msg_includes_all_of("component.yaml:3", "params.an_int", "required"),
validate_error_msg_additional_cli=msg_includes_all_of(
"Field `an_int` is required but not provided"
check_error_msg=msg_includes_all_of(
"component.yaml:3",
"params",
"'an_int' is a required property",
),
)

Expand All @@ -51,13 +58,30 @@ def _validate_error_msg(msg: str) -> None:
),
BASIC_INVALID_VALUE,
BASIC_MISSING_VALUE,
ComponentValidationTestCase(
component_path="validation/simple_asset_invalid_value",
component_type_filepath=None,
should_error=True,
validate_error_msg=msg_includes_all_of(
"component.yaml:5", "params.value", "Input should be a valid string"
),
check_error_msg=msg_includes_all_of(
"component.yaml:5",
"params.value",
"{} is not of type 'string'",
),
),
ComponentValidationTestCase(
component_path="validation/basic_component_extra_value",
component_type_filepath=Path(__file__).parent / "basic_components.py",
should_error=True,
validate_error_msg=msg_includes_all_of(
"component.yaml:7", "params.a_bool", "Extra inputs are not permitted"
),
check_error_msg=msg_includes_all_of(
"component.yaml:3",
"'a_bool' was unexpected",
),
),
ComponentValidationTestCase(
component_path="validation/nested_component_invalid_values",
Expand All @@ -71,6 +95,14 @@ def _validate_error_msg(msg: str) -> None:
"params.nested.baz.a_string",
"Input should be a valid string",
),
check_error_msg=msg_includes_all_of(
"component.yaml:7",
"params.nested.foo.an_int",
"{} is not of type 'integer'",
"component.yaml:12",
"params.nested.baz.a_string",
"{} is not of type 'string'",
),
),
ComponentValidationTestCase(
component_path="validation/nested_component_missing_values",
Expand All @@ -79,8 +111,13 @@ def _validate_error_msg(msg: str) -> None:
validate_error_msg=msg_includes_all_of(
"component.yaml:5", "params.nested.foo.an_int", "required"
),
validate_error_msg_additional_cli=msg_includes_all_of(
"Field `a_string` is required but not provided"
check_error_msg=msg_includes_all_of(
"component.yaml:5",
"params.nested.foo",
"'an_int' is a required property",
"component.yaml:10",
"params.nested.baz",
"'a_string' is a required property",
),
),
ComponentValidationTestCase(
Expand All @@ -94,6 +131,14 @@ def _validate_error_msg(msg: str) -> None:
"component.yaml:15",
"params.nested.baz.another_bool",
),
check_error_msg=msg_includes_all_of(
"component.yaml:5",
"params.nested.foo",
"'a_bool' was unexpected",
"component.yaml:12",
"params.nested.baz",
"'another_bool' was unexpected",
),
),
ComponentValidationTestCase(
component_path="validation/invalid_component_file_model",
Expand All @@ -107,5 +152,13 @@ def _validate_error_msg(msg: str) -> None:
"params",
"Input should be an object",
),
check_error_msg=msg_includes_all_of(
"component.yaml:1",
"type",
"{} is not of type 'string'",
"component.yaml:3",
"params",
"'asdfasdf' is not of type 'object'",
),
),
]
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,19 @@ def load_test_component_defs(name: str) -> Definitions:
)


def load_test_component_project_context() -> CodeLocationProjectContext:
def load_test_component_project_context(include_test: bool = False) -> CodeLocationProjectContext:
components = {}
package_name = "dagster_components.lib"
dc_module = importlib.import_module(package_name)

components = {}
for component in get_registered_component_types_in_module(dc_module):
key = f"dagster_components.{get_component_type_name(component)}"
components[key] = component
packages = ["dagster_components.lib"] + (
["dagster_components.lib.test"] if include_test else []
)
for package_name in packages:
dc_module = importlib.import_module(package_name)

for component in get_registered_component_types_in_module(dc_module):
key = f"dagster_components.{'test.' if package_name.endswith('test') else ''}{get_component_type_name(component)}"
components[key] = component

return CodeLocationProjectContext(
root_path=str(Path(__file__).parent),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: dagster_components.test.simple_asset

params:
asset_key: "test"
value: {}
Original file line number Diff line number Diff line change
@@ -1,22 +1,14 @@
from pathlib import Path

import pytest
from click.testing import CliRunner
from dagster._core.test_utils import new_cwd
from dagster_components.cli import cli
from dagster_components.utils import ensure_dagster_components_tests_import
from pydantic import ValidationError

from dagster_components_tests.integration_tests.validation_tests.test_cases import (
BASIC_INVALID_VALUE,
BASIC_MISSING_VALUE,
from dagster_components.test.test_cases import (
COMPONENT_VALIDATION_TEST_CASES,
ComponentValidationTestCase,
)
from dagster_components.utils import ensure_dagster_components_tests_import
from pydantic import ValidationError

from dagster_components_tests.integration_tests.validation_tests.utils import (
load_test_component_defs_inject_component,
)
from dagster_components_tests.utils import create_code_location_from_components

ensure_dagster_components_tests_import()

Expand Down Expand Up @@ -44,120 +36,3 @@ def test_validation_messages(test_case: ComponentValidationTestCase) -> None:
str(test_case.component_path),
test_case.component_type_filepath,
)


@pytest.mark.parametrize(
"test_case",
COMPONENT_VALIDATION_TEST_CASES,
ids=[str(case.component_path) for case in COMPONENT_VALIDATION_TEST_CASES],
)
def test_validation_cli(test_case: ComponentValidationTestCase) -> None:
"""Tests that the check CLI prints rich error messages when attempting to
load components with errors.
"""
runner = CliRunner()

with create_code_location_from_components(
test_case.component_path, local_component_defn_to_inject=test_case.component_type_filepath
) as tmpdir:
with new_cwd(str(tmpdir)):
result = runner.invoke(
cli,
[
"--builtin-component-lib",
"dagster_components.test",
"check",
"component",
],
catch_exceptions=False,
)
if test_case.should_error:
assert result.exit_code != 0, str(result.stdout)

assert test_case.validate_error_msg
test_case.validate_error_msg(str(result.stdout))

if test_case.validate_error_msg_additional_cli:
test_case.validate_error_msg_additional_cli(str(result.stdout))
else:
assert result.exit_code == 0, str(result.stdout)


@pytest.mark.parametrize(
"scope_check_run",
[True, False],
)
def test_validation_cli_multiple_components(scope_check_run: bool) -> None:
"""Ensure that the check CLI can validate multiple components in a single code location, and
that error messages from all components are displayed.

The parameter `scope_check_run` determines whether the check CLI is run pointing at both
components or none (defaulting to the entire workspace) - the output should be the same in
either case, this just tests that the CLI can handle multiple filters.
"""
runner = CliRunner()

with create_code_location_from_components(
BASIC_MISSING_VALUE.component_path,
BASIC_INVALID_VALUE.component_path,
local_component_defn_to_inject=BASIC_MISSING_VALUE.component_type_filepath,
) as tmpdir:
with new_cwd(str(tmpdir)):
result = runner.invoke(
cli,
[
"--builtin-component-lib",
"dagster_components.test",
"check",
"component",
*(
[
str(
Path("my_location") / "components" / "basic_component_missing_value"
),
str(
Path("my_location") / "components" / "basic_component_invalid_value"
),
]
if scope_check_run
else []
),
],
catch_exceptions=False,
)
assert result.exit_code != 0, str(result.stdout)

assert BASIC_INVALID_VALUE.validate_error_msg and BASIC_MISSING_VALUE.validate_error_msg
BASIC_INVALID_VALUE.validate_error_msg(str(result.stdout))
BASIC_MISSING_VALUE.validate_error_msg(str(result.stdout))


def test_validation_cli_multiple_components_filter() -> None:
"""Ensure that the check CLI filters components to validate based on the provided paths."""
runner = CliRunner()

with create_code_location_from_components(
BASIC_MISSING_VALUE.component_path,
BASIC_INVALID_VALUE.component_path,
local_component_defn_to_inject=BASIC_MISSING_VALUE.component_type_filepath,
) as tmpdir:
with new_cwd(str(tmpdir)):
result = runner.invoke(
cli,
[
"--builtin-component-lib",
"dagster_components.test",
"check",
"component",
str(Path("my_location") / "components" / "basic_component_missing_value"),
],
catch_exceptions=False,
)
assert result.exit_code != 0, str(result.stdout)

assert BASIC_INVALID_VALUE.validate_error_msg and BASIC_MISSING_VALUE.validate_error_msg

BASIC_MISSING_VALUE.validate_error_msg(str(result.stdout))
# We exclude the invalid value test case
with pytest.raises(AssertionError):
BASIC_INVALID_VALUE.validate_error_msg(str(result.stdout))
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from pathlib import Path
from typing import Optional

from dagster._core.definitions.definitions_class import Definitions

Expand All @@ -10,15 +11,16 @@


def load_test_component_defs_inject_component(
src_path: str, local_component_defn_to_inject: Path
src_path: str, local_component_defn_to_inject: Optional[Path]
) -> Definitions:
"""Loads a component from a test component project, making the provided local component defn
available in that component's __init__.py.
"""
with inject_component(
src_path=src_path, local_component_defn_to_inject=local_component_defn_to_inject
) as tmpdir:
context = load_test_component_project_context()
context = load_test_component_project_context(include_test=True)

return build_defs_from_component_path(
path=Path(tmpdir),
registry=context.component_registry,
Expand Down
Loading