[dagster-aws] add PipesEMRClient

[danielgafni]

Summary & Motivation

This PR adds PipesEMRCLient to dagster-aws.

It allows running Spark workloads in ephemeral EMR (EC2 flavor) clusters.

There is no support for submitting steps to existing EMR clusters. It turned out to be pretty hard to support properly since there is no native way to distinguish between steps submitted by our client and other workloads running in the same cluster. We can try to implement this in the future.

Tasks:

• PipesEMRClient implementation
• local testing

Showcase:

How I Tested These Changes

Changelog [New | Bug | Docs]

[dagster-aws] new AWS EMR Dagster Pipes client (dagster_aws.pipes.PipesEMRCLient ) for running and monitoring AWS EMR jobs from Dagster.

[danielgafni]

• [dagster-aws] [docs] add docs for PipesEMRClient #25011
• [dagster-aws] add PipesEMRClient #23998 👈
• [dagster-aws, dagster-pipes] Add PipesS3LogReader #25224 : 1 other dependent PR (#24884 )
• master

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

Join @danielgafni and the rest of your teammates on Graphite

[github-actions]

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-6xg33sq92-elementl.vercel.app
https://08-27-add-pipesemrclient.dagster.dagster-docs.io

Direct link to changed pages:

[schrockn]

Can we separate the new client from the addition of a base class? The base class is a pretty big decision and we'd want to refactor the other clients.

[schrockn]

For example I would want to see what a version looks like that extracts common functionality into functions and we compose, rather than inherit.

[danielgafni]

Yeah sure, I just wanted to know if this was a good idea at all.
I'll make a separate PR if you think it's worth trying out.

[danielgafni]

To be clear, I don't want to use this base class across absolutely all pipes clients. It's just for AWS (at least for now).

[danielgafni]

Done here: #24042

[graphite-app]

Graphite Automations

"docs-beta - Assign Reviewers" took an action on this PR • (08/29/24)

2 reviewers were added and 1 label was added to this PR based on Pedram Navid's automation.

[schrockn]

Oh maybe there was a miscommunication here. What I was thinking is that you would add PipesEMRClient without the base class so that we get the integration done, but then we can refactor it to share code with the other clients. I think the refactor should be down with at minimum 2 examples applied so we aren't overfitting it to a particular use case.

[smackesey]

Inline comments and needs tests. I think this can be merged independently of the downstack one where you add the key argument to PipesS3MessageReader, so you should reverse the order of the PRs. PipesS3LogReader can be included in this one or a separate PR if necessary.

[smackesey]

To your queue (re-request review when this is ready)

[danielgafni]

downstack one where you add the key argument to PipesS3MessageReader

It's the other way: in order to merge this PR we only need PipesS3LogReader; PipesS3MessageReader's key argument didn't end up being useful for EMR; but it can be useful for EMR Serverless. I'll make a PR using it in EMR Serverless.

The current PR graph is correct.

[smackesey]

It's the other way: in order to merge this PR we only need PipesS3LogReader; PipesS3MessageReader's key argument didn't end up being useful for EMR; but it can be useful for EMR Serverless. I'll make a PR using it in EMR Serverless.

I think the comment you're responding to was made before you reordered the PRs. Yes, looks like the current stack order is correct.

[danielgafni]

needs tests

@smackesey I think we can't test anything valuable right now (best we can do is to check Pipes params inkection), because testing setup for EMR is quite complicated and requires real AWS infra.

I have some manual tests in this PR. I just had a call with @gibsondan and we agreed on adding infra related tests to the internal repo.

I will start working on this later this week. This isn't just EMR's problem, other Pipes clients like Glue, EMR Serverless, ECS, and probably Databricks also require this.

For now I suggest we merge this PR without tests.

[smackesey]

Please invoke on_launched then feel free to merge

stale and now approved by @smackesey