[dagster-aws] delete dummy Pipes classes

[danielgafni]

Summary & Motivation

Deleting dummy Pipes classes from AWS Pipes. These classes didn't provide any functionality and their introduction was questionable from the start.

Context:

• [dagster-aws] add Pipes cloudwatch message reader #23353 (comment)
• [dagster-aws] add PipesGlueClient #22968

I decided to keep PipesGlueLambdaEventContextInjector, because unlike the the other classes, it's actually used for a unique purpose: injecting variables into Lambda event input (it might be a bit confusing because it inherits from PipesEnvContextInjector but doesn't actually do anything with environment variables).

@schrockn let me know if you think if it makes sense

How I Tested These Changes

Nothing was really using these classes

[danielgafni]

• [dagster-aws] delete dummy Pipes classes #23441 👈
• [dagster-aws] add Pipes cloudwatch message reader #23353 : 2 other dependent PRs (#23354 , #23440 )
• master

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

Join @danielgafni and the rest of your teammates on Graphite

[schrockn]

Curious to hear what @cmpadden thinks, but I think these wrapper classes actually obscure what is actually happening. If we imagine the implementations diverging more, that is another thing. But I think we should be explicit that we are fetching logs from CloudWatch or injecting context via S3 in a standardized way in these cases.

[github-actions]

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-kdodoyv9t-elementl.vercel.app
https://08-06-delete-dummy-classes.dagster.dagster-docs.io

Direct link to changed pages:

• https://dagster-docs-kdodoyv9t-elementl.vercel.app
  https://08-06-delete-dummy-classes.dagster.dagster-docs.io/concepts/dagster-pipes/aws-glue

[cmpadden]

Curious to hear what @cmpadden thinks, but I think these wrapper classes actually obscure what is actually happening. If we imagine the implementations diverging more, that is another thing. But I think we should be explicit that we are fetching logs from CloudWatch or injecting context via S3 in a standardized way in these cases.

I'm in agreement -- I don't see the benefit of the wrapper classes in their current form. It's more clear to have the full context of where logs are being fetched without the added layer of abstraction.

[danielgafni]

Seems like this is good to go then?

@schrockn any idea why graphite doesn't propagate the pyright error fix from #23353 to this branch? I tried gt restack but it doesn't seem to help

[schrockn]

No idea on your graphite issue.

Were these public or in docs? Do we need to go through a deprecation cycle?

[cmpadden]

Seems like this is good to go then?

@schrockn any idea why graphite doesn't propagate the pyright error fix from #23353 to this branch? I tried gt restack but it doesn't seem to help

You may need to do a gt get -- I'm by no means a graphite guru, but that's been helping me with rebase issues.

[danielgafni]

Were these public or in docs? Do we need to go through a deprecation cycle?

They weren't.

Also, the Glue client is marked as Experimental.

[schrockn]

Great!