dagster icon indicating copy to clipboard operation
dagster copied to clipboard

Make the execute and execute_script_file utilities in dagster_shell part of the public API

Open fahadkh opened this issue 2 years ago • 2 comments

Summary & Motivation

A draft PR to show an example of how we could resolve https://github.com/dagster-io/dagster/issues/8879.

Currently, dagster_shell exposes several public APIs for creating shell command/script ops. These work fine, but often we want to connect op input data or data generated within an op to a shell command rather than just running the shell command alone.

This is especially necessary when using something like the k8s executor which runs each op in a separate, isolated pod (meaning we don't have shared filesystem either).

How I Tested These Changes

Updated the docs, and loaded the doc site locally to verify that it is generated properly.

Screenshots

image

image

fahadkh avatar Jul 14 '22 16:07 fahadkh

@fahadkh is attempting to deploy a commit to the Elementl Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar Jul 14 '22 16:07 vercel[bot]

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

2 Ignored Deployments
Name Status Preview Updated
dagit-storybook ⬜️ Ignored (Inspect) Aug 11, 2022 at 9:08PM (UTC)
dagster-oss-cloud-consolidated ⬜️ Ignored (Inspect) Aug 11, 2022 at 9:08PM (UTC)

vercel[bot] avatar Jul 14 '22 16:07 vercel[bot]

Thanks for the contribution! Overall, this looks good to me, but there are a few formatting issues left here.

I think everything should be possible to clear up pretty quickly with make isort and make black (see: https://docs.dagster.io/community/contributing#developing-dagster), as well as tox -e pylint in the libraries/dagster-shell directory.

It might also be worth rebasing before you do these, as there have been a bunch of changes recently (I don't think they'll affect this PR, but better to be safe).

@OwenKephart Thanks for the links and guidance!

fahadkh avatar Aug 11 '22 21:08 fahadkh