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] add check definitions command #27873

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

benpankow
Copy link
Member

@benpankow benpankow commented Feb 14, 2025

Summary

Introduces dg definitions validate, which calls the underlying dagster definitions validate with appropriate parameters for the current deployment or code location project. Largely cribbed from dg dev.

We could rename this to just dg validate or dg definitions-validate to avoid adding another top-level CLI.

dagster ~/repos/components_demo/my-deployment-2
$ dg definitions validate

Using ephemeral dagster definitions validate
2025-02-14 16:02:57 -0800 - dagster - INFO - Using temporary directory /Users/ben/repos/components_demo/my-deployment-2/tmpaxze6pxy for storage. This will be removed when dagster definitions validate exits.
2025-02-14 16:02:57 -0800 - dagster - INFO - To persist information across sessions, set the environment variable DAGSTER_HOME to a directory to use.
2025-02-14 16:02:58 -0800 - dagster.code_server - INFO - Starting Dagster code server for file /Users/ben/repos/components_demo/my-deployment-2/code_locations/analytics/analytics/definitions.py in process 55388
2025-02-14 16:02:58 -0800 - dagster - WARNING - /Users/ben/repos/components_demo/my-deployment-2/code_locations/analytics/.venv/lib/python3.12/site-packages/dagster/_core/utils.py:125: UserWarning: Found version mismatch between `dagster` (1.10.1) expected library version (0.26.1) and `dagster-sling` (1!0+dev).
  warnings.warn(message)

2025-02-14 16:02:58 -0800 - dagster.code_server - INFO - Started Dagster code server for file /Users/ben/repos/components_demo/my-deployment-2/code_locations/analytics/analytics/definitions.py in process 55388
2025-02-14 16:02:59 -0800 - dagster.code_server - INFO - Starting Dagster code server for file /Users/ben/repos/components_demo/my-deployment-2/code_locations/jaffle-platform/jaffle_platform/definitions.py in process 55392
2025-02-14 16:03:00 -0800 - dagster.code_server - INFO - Started Dagster code server for file /Users/ben/repos/components_demo/my-deployment-2/code_locations/jaffle-platform/jaffle_platform/definitions.py in process 55392
INFO:dagster.code_server:Started Dagster code server for file /Users/ben/repos/components_demo/my-deployment-2/code_locations/jaffle-platform/jaffle_platform/definitions.py in process 55392
2025-02-14 16:03:00 -0800 - dagster - INFO - Validation successful for code location analytics.
2025-02-14 16:03:00 -0800 - dagster - INFO - Validation successful for code location jaffle-platform.
2025-02-14 16:03:00 -0800 - dagster - INFO - All code locations passed validation.

There's also a fair amount of logspew on the dagster definitions validate end that a stacked PR can clean up.

Test Plan

Some brief test cases, tested manually.

@benpankow
Copy link
Member Author

benpankow commented Feb 14, 2025

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

@benpankow benpankow changed the base branch from benpankow/dg-check-watch to graphite-base/27873 February 14, 2025 23:44
@benpankow benpankow force-pushed the benpankow/dg-definitions-validate branch from ee23a34 to 31f10d3 Compare February 14, 2025 23:44
@benpankow benpankow changed the base branch from graphite-base/27873 to master February 14, 2025 23:44
Copy link
Collaborator

@smackesey smackesey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely functionality we want but I am strongly opposed to dg definitions validate as the name. Explained why in this doc which I posted in the channel and which touches on questions I'd like to settle before merging this: https://www.notion.so/dagster/Proposal-project-deployment-code-location-and-the-command-structure-of-dg-19c18b92e46280ad9d72e54cafdeec8e?pvs=4

Summary of that doc wrt this PR is that I think we should have a top-level check command and this should be dg check definitions (alongside dg check yaml).

@benpankow
Copy link
Member Author

This is definitely functionality we want but I am strongly opposed to dg definitions validate as the name. Explained why in this doc which I posted in the channel and which touches on questions I'd like to settle before merging this: https://www.notion.so/dagster/Proposal-project-deployment-code-location-and-the-command-structure-of-dg-19c18b92e46280ad9d72e54cafdeec8e?pvs=4

Totally agree, I am unhappy with the command name as-written in this PR. Will wait for resolution on that doc.

@benpankow benpankow force-pushed the benpankow/dg-definitions-validate branch from 9b89cf9 to 2ac626b Compare February 21, 2025 20:01
Copy link
Collaborator

@smackesey smackesey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can land this now as dg check definitions per discussion.

@benpankow benpankow changed the title [dg] add definitions validate command [dg] add check definitions command Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants