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

benpankow · 2025-01-18T18:14:05Z

Summary

Moves the dg check implementation to use jsonschema validation, eschewing the previous approach which would actually load each component using the dagster-components machinery.

Relies on #27285 to load schemas for local component types, in addition to our current cache for global component types & their schemas.

A few follow-ups to make this faster:

Cache local component schemas, should be easy enough to do so in a stacked PR.

Test Plan

Shared test cases with the existing validation tests, moved over from dagster-components check tests.

vercel · 2025-01-18T18:14:10Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
dagster-docs-legacy	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 18, 2025 6:16pm

benpankow · 2025-01-18T18:14:22Z

[dg] Move check impl to lighter weight JSON validation #27214 : 2 dependent PRs (#27291 , #27335 ) 👈 (View in Graphite)
[dagster-components cli] add dagster-components list local-components-types #27285
Mirror YAML parsing utils in dagster-dg #27294
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

python_modules/libraries/dagster-dg/dagster_dg/cli/component.py

OwenKephart

seems like a reasonable implementation, have some comments though

also might be nice to see a sample output somewhere

OwenKephart · 2025-01-22T23:19:34Z

python_modules/libraries/dagster-dg/dagster_dg/cli/component.py

+    component_contents_by_dir = {}
+    local_components = set()
+    for component_dir in (
+        dg_context.root_path / dg_context.root_package_name / "components"


you can simplify this a bit with:

for component_path in component_path.rglob("component.yaml"): ...

that'll handle walking the directory for you and finding all the component.yaml files. You can probably also use rglob to filter for things in the resolved paths, although not sure if that's that much better

I bias a little towards the iterdir approach just to mirror 1:1 how we actually pick up components elsewhere in the codebase - we end up needing to find both the component yaml and the component dir anyway so it doesn't save a ton of space

seems like a case where a common utility would be nice here -- nbd for now

OwenKephart · 2025-01-22T23:22:27Z

python_modules/libraries/dagster-dg/dagster_dg/cli/component.py

@@ -259,10 +272,76 @@ def component_check_command(
    **global_options: object,
 ) -> None:
    """Check component files against their schemas, showing validation errors."""
+    resolved_paths = [Path(path).absolute() for path in paths]
+    top_level_component_validator = Draft202012Validator(schema=COMPONENT_FILE_SCHEMA)


do we need to use a specific Validator class like this instead of just jsonschema.validate()? This class name seems kinda unfriendly

validate only raises an exception with the first error; we need a validator in order to access iter_errors

python_modules/libraries/dagster-dg/dagster_dg/cli/component.py

OwenKephart · 2025-01-22T23:28:04Z

python_modules/libraries/dagster-dg/dagster_dg/component.py

+class LocalComponentTypes:
+    @classmethod
+    def from_dg_context(cls, dg_context: "DgContext", component_dirs: Sequence[Path]):
+        # TODO: cache
+
+        raw_local_component_data = dg_context.external_components_command(
+            ["list", "local-component-types", *component_dirs]
+        )
+        local_component_data = json.loads(raw_local_component_data)
+
+        components_by_path = {str(path): {} for path in component_dirs}
+        for entry in local_component_data:
+            components_by_path[entry["directory"]][entry["key"]] = RemoteComponentType(
+                **entry["metadata"]
+            )
+        return cls(components_by_path)
+
+    def __init__(self, components_by_path: dict[str, dict[str, RemoteComponentType]]):
+        self._components_by_path = copy.copy(components_by_path)
+
+    def get(self, path: Path, key: str) -> RemoteComponentType:
+        return self._components_by_path[str(path)][key]


I feel like the name of this class is pretty confusing -- it's called LocalComponentTypes but it's storing a bunch of RemoteComponentTypes

It does sort of feel like we should just have a param for RemoteComponentRegistry to load additional local component types. Like from_dg_context having an optional additional_component_dirs parameter.

That would simplify code that uses this stuff too -- you don't have to juggle two separate objects around and check which one you should be using

Yeah I agree the name sucks. I do think it's somewhat useful that it's separate in that

it's not comprehensive, e.g. a single instance represents a set of local component types from a few components, not necessarily the whole code location

you may only want to load it in some scenarios, in others you only want to load the global RemoteComponentRegistry component type list

optional additional_component_dirs

This would work, but would need a refactor of RemoteComponentRegistry to no longer use component type as a unique key, since local components can have name overlap (since they're 'scoped' to their folder)

i see what you mean about uniqueness -- I guess just more fundamentally I think "bag of remote components that we want to do stuff with" should be a single object. this code feels pretty brittle and painful to work with as time goes on.

what other cases are you imagining where we'd want only the LocalComponents?

i could be convinced that we could stick with this sort of thing (ideally with a rename) until a new usecase comes along for us to refactor based off of, just feels like this pattern shouldn't stay around forever

might be a matter of subclassing or something

yeah let me feel out what it would look like to rewrite the existing remote components class

did a restructure, I'm happier with this approach

python_modules/libraries/dagster-dg/dagster_dg/cli/component.py

q

python_modules/libraries/dagster-dg/dagster_dg/cli/component.py

OwenKephart

I think we'll want to do another pass on the local vs. global component stuff, but given that we're not sure of what the end state of that will be, I think this is good for now

vercel bot deployed to Preview January 18, 2025 18:16 View deployment

benpankow force-pushed the benpankow/check-lw branch from b0de1ac to 77812d5 Compare January 22, 2025 20:35

benpankow changed the base branch from master to benpankow/list-local-component-types January 22, 2025 20:35

benpankow mentioned this pull request Jan 22, 2025

[dagster-components cli] add dagster-components list local-components-types #27285

Merged

benpankow force-pushed the benpankow/check-lw branch 2 times, most recently from 725098b to 1956009 Compare January 22, 2025 22:31

benpankow requested review from smackesey and OwenKephart January 22, 2025 22:31

benpankow marked this pull request as ready for review January 22, 2025 22:32

benpankow requested a review from schrockn January 22, 2025 22:32

benpankow force-pushed the benpankow/list-local-component-types branch from 7c52fad to 4eea8a2 Compare January 22, 2025 22:51

benpankow force-pushed the benpankow/check-lw branch from 1956009 to a4d6f07 Compare January 22, 2025 22:51

benpankow mentioned this pull request Jan 22, 2025

remove yaml parsing deps on dagster from dg #27291

Closed

graphite-app bot reviewed Jan 22, 2025

View reviewed changes

python_modules/libraries/dagster-dg/dagster_dg/cli/component.py Outdated Show resolved Hide resolved

OwenKephart previously requested changes Jan 22, 2025

View reviewed changes

graphite-app bot reviewed Jan 22, 2025

View reviewed changes

python_modules/libraries/dagster-dg/dagster_dg/cli/component.py Outdated Show resolved Hide resolved

benpankow force-pushed the benpankow/list-local-component-types branch from 4eea8a2 to 185efb7 Compare January 23, 2025 00:20

benpankow force-pushed the benpankow/check-lw branch from 584c859 to 00df599 Compare January 23, 2025 00:20

benpankow mentioned this pull request Jan 23, 2025

Mirror YAML parsing utils in dagster-dg #27294

Merged

benpankow force-pushed the benpankow/list-local-component-types branch from 185efb7 to 1ed28d0 Compare January 23, 2025 15:15

benpankow force-pushed the benpankow/check-lw branch from 00df599 to c9d5824 Compare January 23, 2025 15:15

benpankow requested a review from OwenKephart January 23, 2025 15:39

benpankow force-pushed the benpankow/check-lw branch from e269c1d to a873ee3 Compare January 23, 2025 18:48

Base automatically changed from benpankow/list-local-component-types to master January 23, 2025 19:09

benpankow force-pushed the benpankow/check-lw branch from 7cf47f1 to 15a9207 Compare January 23, 2025 19:10

graphite-app bot reviewed Jan 23, 2025

View reviewed changes

python_modules/libraries/dagster-dg/dagster_dg/cli/component.py Show resolved Hide resolved

benpankow force-pushed the benpankow/check-lw branch from 15a9207 to ea0a069 Compare January 23, 2025 23:12

benpankow mentioned this pull request Jan 23, 2025

[dg] Cache local component type schemas to speed up repeated dg check calls #27335

Merged

OwenKephart approved these changes Jan 24, 2025

View reviewed changes

benpankow added 10 commits January 23, 2025 16:52

[dg] Move check impl to lighter weight JSON validation

ceda3d2

ruff

6720407

spin

6bb1d44

move test cases to components module

359fb73

test

8ae934e

fix last test

62abab6

reorder

c397357

reorder

161243b

ok

ef35e7a

pywrong

3dc9f35

benpankow force-pushed the benpankow/check-lw branch from 34feb2b to 3dc9f35 Compare January 24, 2025 00:53

benpankow merged commit 1c2821a into master Jan 24, 2025
5 checks passed

benpankow deleted the benpankow/check-lw branch January 24, 2025 17:09

benpankow mentioned this pull request Jan 24, 2025

[dg check] Gracefully error when component type isnt found #27357

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

benpankow commented Jan 18, 2025 •

edited

Loading

vercel bot commented Jan 18, 2025 •

edited

Loading

benpankow commented Jan 18, 2025 •

edited

Loading

OwenKephart left a comment

OwenKephart Jan 22, 2025

OwenKephart Jan 22, 2025

benpankow Jan 23, 2025

OwenKephart Jan 23, 2025

OwenKephart Jan 22, 2025

benpankow Jan 23, 2025

OwenKephart Jan 23, 2025

OwenKephart Jan 22, 2025

benpankow Jan 22, 2025 •

edited

Loading

OwenKephart Jan 23, 2025

benpankow Jan 23, 2025

benpankow Jan 23, 2025

OwenKephart left a comment

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

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

Conversation

benpankow commented Jan 18, 2025 • edited Loading

Summary

Test Plan

vercel bot commented Jan 18, 2025 • edited Loading

benpankow commented Jan 18, 2025 • edited Loading

OwenKephart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpankow Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwenKephart left a comment

Choose a reason for hiding this comment

benpankow commented Jan 18, 2025 •

edited

Loading

vercel bot commented Jan 18, 2025 •

edited

Loading

benpankow commented Jan 18, 2025 •

edited

Loading

benpankow Jan 22, 2025 •

edited

Loading