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

add pool graphql fields for runs #26964

Open
wants to merge 1 commit into
base: prha/hoist_run_queue_config
Choose a base branch
from

Conversation

prha
Copy link
Member

@prha prha commented Jan 9, 2025

Summary & Motivation

Makes pool information available for the containing runs, to provide debugging information on the Run view, potentially the Run table.

How I Tested These Changes

BK

@prha prha changed the title run graphql add pool graphql fields for runs Jan 9, 2025
Copy link

github-actions bot commented Jan 9, 2025

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-3gckb8g3x-elementl.vercel.app
https://prha-run-pools-graphql.core-storybook.dagster-docs.io

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

@prha prha force-pushed the prha/concurrency_granularity branch from d217d12 to 72da007 Compare January 10, 2025 01:18
@prha prha force-pushed the prha/run_pools_graphql branch from 4dacceb to a8ac3e7 Compare January 10, 2025 01:18
@prha prha force-pushed the prha/concurrency_granularity branch from 72da007 to 8782d96 Compare January 10, 2025 23:23
@prha prha force-pushed the prha/run_pools_graphql branch from a8ac3e7 to 3d16333 Compare January 10, 2025 23:23
@prha prha force-pushed the prha/concurrency_granularity branch from 8782d96 to 5c83e2c Compare January 16, 2025 00:03
@prha prha force-pushed the prha/run_pools_graphql branch from 3d16333 to a4ee6b4 Compare January 16, 2025 00:03
@prha prha force-pushed the prha/concurrency_granularity branch from 5c83e2c to 9e5e36d Compare January 16, 2025 00:19
@prha prha force-pushed the prha/run_pools_graphql branch from a4ee6b4 to ebd3269 Compare January 16, 2025 00:20
@prha prha force-pushed the prha/concurrency_granularity branch from 9e5e36d to eb04b1b Compare January 16, 2025 01:11
@prha prha force-pushed the prha/run_pools_graphql branch from ebd3269 to a81756e Compare January 16, 2025 01:11
@prha prha force-pushed the prha/concurrency_granularity branch from eb04b1b to d80e191 Compare January 16, 2025 20:23
@prha prha force-pushed the prha/run_pools_graphql branch from a81756e to b202de6 Compare January 16, 2025 20:23
@prha prha force-pushed the prha/concurrency_granularity branch from d80e191 to e5ad4c3 Compare January 17, 2025 01:08
@prha prha force-pushed the prha/run_pools_graphql branch from b202de6 to 46e8b90 Compare January 17, 2025 01:08
@prha prha force-pushed the prha/concurrency_granularity branch from e5ad4c3 to 111c97c Compare January 17, 2025 02:27
@prha prha force-pushed the prha/run_pools_graphql branch from 46e8b90 to 591be98 Compare January 17, 2025 02:27
@prha prha force-pushed the prha/concurrency_granularity branch from 111c97c to 4e2eb36 Compare January 17, 2025 02:55
@prha prha force-pushed the prha/run_pools_graphql branch from 591be98 to 1ef1cac Compare January 17, 2025 02:55
@prha prha requested a review from OwenKephart January 17, 2025 21:18
@prha prha marked this pull request as ready for review January 17, 2025 21:18
Comment on lines +626 to +630
def resolve_allConcurrencyKeys(self, graphene_info: ResolveInfo):
if not self.dagster_run.run_op_concurrency:
return None

return list(self.dagster_run.run_op_concurrency.all_pools)
Copy link

Choose a reason for hiding this comment

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

The schema defines allConcurrencyKeys as a non-null array of strings ([String!]), but this resolver returns None when run_op_concurrency is not present. To maintain type consistency, consider returning an empty list instead:

return [] if not self.dagster_run.run_op_concurrency else list(self.dagster_run.run_op_concurrency.all_pools)

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

@@ -389,6 +389,7 @@ class GrapheneRun(graphene.ObjectType):
hasDeletePermission = graphene.NonNull(graphene.Boolean)
hasConcurrencyKeySlots = graphene.NonNull(graphene.Boolean)
rootConcurrencyKeys = graphene.List(graphene.NonNull(graphene.String))
allConcurrencyKeys = graphene.List(graphene.NonNull(graphene.String))
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a net-new field should we just call this allPools from the outset?

Copy link
Contributor

@OwenKephart OwenKephart left a comment

Choose a reason for hiding this comment

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

note naming comment

@prha prha force-pushed the prha/run_pools_graphql branch from 1ef1cac to b58d918 Compare January 17, 2025 22:30
@prha prha requested a review from neverett as a code owner January 17, 2025 22:30
@prha prha changed the base branch from prha/concurrency_granularity to prha/hoist_run_queue_config January 17, 2025 22:30
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