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.

Sign up for GitHub

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

Jump to bottom

[dagster-aws] add Pipes ECS client #23568

Merged
merged 2 commits into from
Aug 28, 2024
Merged

[dagster-aws] add Pipes ECS client #23568

merged 2 commits into from
Aug 28, 2024

Conversation

danielgafni
Copy link
Contributor

@danielgafni danielgafni commented Aug 10, 2024 •
edited
Loading

Summary & Motivation

Adds Dagster Pipes ECS client

How I Tested These Changes

  • add moto test for execution
  • add moto test for interruption

@vercel Vercel
Copy link

vercel bot commented Aug 10, 2024

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

Name Status Preview Comments Updated (UTC)
dagster-docs-next ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 10, 2024 2:22pm

@graphite-app graphite-app bot requested a review from erinkcochran87 August 10, 2024 14:22
@graphite-app graphite-app bot added the area: docs Related to documentation in general label Aug 10, 2024
@danielgafni danielgafni marked this pull request as draft August 10, 2024 14:22
@danielgafni danielgafni changed the base branch from master to 08-08-refactor_dagster_aws.pipes_module_into_a_package August 10, 2024 14:23
@danielgafni danielgafni removed the request for review from erinkcochran87 August 10, 2024 14:23
@danielgafni danielgafni mentioned this pull request Aug 10, 2024
Merged
@danielgafni Graphite App
Copy link
Contributor Author

danielgafni commented Aug 10, 2024 •
edited
Loading

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

Join @danielgafni and the rest of your teammates on Graphite Graphite

This was referenced Aug 10, 2024
Merged
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 10, 2024 •
edited
Loading

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-jozrfr8mu-elementl.vercel.app
https://08-09--dagster-aws-add-ecs-pipes.dagster.dagster-docs.io

Direct link to changed pages:

@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch from ef6bb27 to 945a5d3 Compare August 16, 2024 13:25
@danielgafni danielgafni force-pushed the 08-08-refactor_dagster_aws.pipes_module_into_a_package branch from 1537d9f to 96640d4 Compare August 16, 2024 14:04
@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch from 945a5d3 to de67177 Compare August 16, 2024 14:05
@danielgafni danielgafni marked this pull request as ready for review August 16, 2024 14:08
@danielgafni danielgafni force-pushed the 08-08-refactor_dagster_aws.pipes_module_into_a_package branch from 96640d4 to f2197f5 Compare August 16, 2024 15:47
@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch from de67177 to a1864e5 Compare August 16, 2024 15:47
@danielgafni danielgafni force-pushed the 08-08-refactor_dagster_aws.pipes_module_into_a_package branch from f2197f5 to 9339f59 Compare August 16, 2024 15:54
@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch from a1864e5 to 592d340 Compare August 16, 2024 15:55
@danielgafni danielgafni force-pushed the 08-08-refactor_dagster_aws.pipes_module_into_a_package branch from 9339f59 to 633d4c6 Compare August 16, 2024 16:12
@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch from 592d340 to 76314fe Compare August 16, 2024 16:12
@danielgafni danielgafni force-pushed the 08-08-refactor_dagster_aws.pipes_module_into_a_package branch from 633d4c6 to 5ada018 Compare August 16, 2024 21:30
@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch from 76314fe to a76eaa6 Compare August 16, 2024 21:30
@danielgafni danielgafni force-pushed the 08-08-refactor_dagster_aws.pipes_module_into_a_package branch from 5ada018 to 2b5d54e Compare August 16, 2024 21:32
@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch from a76eaa6 to 38144fb Compare August 16, 2024 21:32
Base automatically changed from 08-08-refactor_dagster_aws.pipes_module_into_a_package to master August 17, 2024 17:10
@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch from 38144fb to ed9a6a4 Compare August 19, 2024 21:16
@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch from 1659b92 to 2ad2041 Compare August 23, 2024 13:56
@danielgafni
Copy link
Contributor Author

danielgafni commented Aug 23, 2024 •
edited
Loading

Checked boto3-stubs. They provide the TypedDict we want! Pushing a commit using it. @schrockn what do you think about this approach now?

  • we don't have to maintain the TypedDict, it comes from boto3-stubs instead
  • it allows passing extra kwargs just in case, it doesn't affect runtime failures
  • it provides type-checking capabilities and IDE help

I think it's great. We should use this approach for other similar places in our codebase.

@danielgafni danielgafni mentioned this pull request Aug 23, 2024
Closed
@danielgafni danielgafni requested a review from schrockn August 23, 2024 14:53
schrockn
schrockn requested changes Aug 23, 2024
View reviewed changes
Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

Almost there

python_modules/libraries/dagster-aws/dagster_aws/pipes/clients/ecs.py Outdated Show resolved Hide resolved
python_modules/libraries/dagster-aws/dagster_aws/pipes/clients/ecs.py Outdated Show resolved Hide resolved
@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch 2 times, most recently from d717e3b to 96aa958 Compare August 26, 2024 10:59
@danielgafni danielgafni requested a review from schrockn August 26, 2024 11:25
@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch 3 times, most recently from 27640c8 to f637780 Compare August 27, 2024 07:58
@danielgafni danielgafni mentioned this pull request Aug 27, 2024
Merged
@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch 3 times, most recently from 9739823 to 7f389db Compare August 27, 2024 10:29
@danielgafni danielgafni mentioned this pull request Aug 27, 2024
Merged
@danielgafni
Copy link
Contributor Author

@schrockn The addition of boto3-stubs-lite caused pyright to start complaining about various boto3 calls in dagster-aws which were previously considered untyped. I made a separate PR to ignore these errors: #23954

@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch from 7f389db to eac4e63 Compare August 28, 2024 07:52
@danielgafni danielgafni mentioned this pull request Aug 28, 2024
Merged
2 tasks
@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch 2 times, most recently from a0f68ca to 25fb13f Compare August 28, 2024 16:15
@danielgafni Graphite App
Copy link
Contributor Author

danielgafni commented Aug 28, 2024 •
edited
Loading

Merge activity

  • Aug 28, 1:32 PM EDT: @danielgafni started a stack merge that includes this pull request via Graphite.
  • Aug 28, 1:33 PM EDT: Graphite rebased this pull request as part of a merge.
  • Aug 28, 1:34 PM EDT: @danielgafni merged this pull request with Graphite.

@danielgafni
add ECS client
159bb97
@danielgafni
[dagster-aws] ignore pyright errors with boto3-stubs (#23954)
2782178 
## Summary & Motivation

Ignore pyright errors in `dagster-aws` which popped out after the
introduction of `boto3-stubs-lite`.

I suggest we slowly remove these ignore comments and fix the code for
pyright to pass over time.

I made a script to do it automatically. 

## How I Tested These Changes

pyright should be passing now

## Changelog [New | Bug | Docs]

> NOCHANGELOG
@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch from 25fb13f to 2782178 Compare August 28, 2024 17:33
@danielgafni danielgafni merged commit 87e6397 into master Aug 28, 2024
1 of 2 checks passed
@danielgafni danielgafni deleted the 08-09-_dagster-aws_add_ecs_pipes branch August 28, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schrockn schrockn schrockn approved these changes

Assignees
No one assigned
Labels
area: docs Related to documentation in general
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@danielgafni @schrockn