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

[4/n] [RFC] add launch multiple runs backend functionality #25880

Open
wants to merge 1 commit into
base: dliu27/add-manual-tick-to-automation-rows
Choose a base branch
from

Conversation

dliu27
Copy link
Contributor

@dliu27 dliu27 commented Nov 12, 2024

Linear: https://linear.app/dagster-labs/issue/FE-659/add-launch-all-backend-functionality

Summary & Motivation

Add a backend GraphQL mutation to handle launching multiple runs

How I Tested These Changes

pytest python_modules/dagster-graphql/dagster_graphql_tests/graphql/test_run_launcher.py
50 passed, 18 warnings in 206.16s (0:03:26)

Changelog

Insert changelog entry or delete this section.

Copy link

github-actions bot commented Nov 12, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-dslt4lvjq-elementl.vercel.app
https://dliu27-launch-all-backend-actual.core-storybook.dagster-docs.io

Built with commit 7e104b4.
This pull request is being automatically deployed with vercel-action

@dliu27 dliu27 changed the title [4/n] add launch multiple runs backend functionality [4/n] [RFC] add launch multiple runs backend functionality Nov 12, 2024
@dliu27 dliu27 force-pushed the dliu27/add-manual-tick-to-automation-rows branch from 9e8213d to 4406df6 Compare November 13, 2024 18:13
@dliu27 dliu27 force-pushed the dliu27/launch-all-backend-actual branch from db05f4b to 7e104b4 Compare November 13, 2024 18:17
@@ -73,17 +74,22 @@ class Meta:

class GrapheneLaunchRunResult(graphene.Union):
class Meta:
from dagster_graphql.schema.backfill import pipeline_execution_error_types
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

and why was it there in the first place 🤔

Comment on lines +141 to +146
for run_id in run_ids:
result = execute_dagster_graphql(
context=graphql_context, query=RUN_QUERY, variables={"runId": run_id}
)
assert result.data["pipelineRunOrError"]["__typename"] == "Run"
assert result.data["pipelineRunOrError"]["status"] == "SUCCESS"
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure we need this? I guess it's just asserting that the runs would have succeeded also if they were run individually but I think we can just assume that is the case given the mock setup.

name = "LaunchMultipleRunsMutation"

@capture_error
@require_permission_check(Permissions.LAUNCH_PIPELINE_EXECUTION)
Copy link
Contributor

Choose a reason for hiding this comment

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

This permission check here can make the whole mutation fail, and in this case the output would be a single GrapheneUnauthorizedError so I believe your output should be a union of GrapheneLaunchMultipleRunsResult and GrapheneUnauthorizedError and also GraphenePythonError since we're using @capture_error.

Copy link
Contributor

@salazarm salazarm left a comment

Choose a reason for hiding this comment

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

Fix the output type to be a union of GraphenePythonError, GrapheneUnauthorizedError and GrapheneLaunchMultipleRunsResult

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