Add dagster-elt #16681

PedramNavid · 2023-09-21T04:03:21Z

Summary & Motivation

This adds the dagster-elt integration which aims to be a set of embedded ELT/ETL tooling to help data engineers with common ingestion/transform tasks such as ingesting data to and from databases, warehouses, and files.

This first iteration is a wrapper around the sling package. Sling is a lightweight CLI tool that allows users to sync data from a source/destination without the extra burden of heavyweight tooling that other solutions require.

How I Tested These Changes

Unit tests, local testing.

python_modules/libraries/dagster-elt/dagster_elt/asset_defs.py

python_modules/libraries/dagster-elt/README.md

python_modules/libraries/dagster-elt/dagster_elt/__init__.py

python_modules/libraries/dagster-elt/dagster_elt/asset_defs.py

petehunt · 2023-09-22T14:37:40Z

Can you confirm that this library works on Serverless?

schrockn · 2023-09-22T15:21:17Z

I want to surface the concern I have (which @sryza also shares and flagged yesterday) that the dagster-elt without any reference to sling as the underlying technology is pretty problematic, as there is 1) more than one underlying tech to do ELT and 2) we don't want to overpromise our capabilties.

I fully support having elt in the same somewhere, but I need to be convinced that dagster-elt is the right thing to GTM with w/r/t to alternatives.

schrockn

Req'ing changes for q mgmt

python_modules/libraries/dagster-elt/dagster_elt/ops.py

python_modules/libraries/dagster-elt/dagster_elt/asset_defs.py

python_modules/libraries/dagster-elt/dagster_elt/resources.py

petehunt · 2023-09-25T00:01:46Z

I want to surface the concern I have (which @sryza also shares and flagged yesterday) that the dagster-elt without any reference to sling as the underlying technology is pretty problematic, as there is 1) more than one underlying tech to do ELT and 2) we don't want to overpromise our capabilties.

Let's call it dagster_embedded_elt?

PedramNavid · 2023-09-27T17:46:59Z

Current dependencies on/for this PR:

master
- PR Add dagster-elt #16681 👈

This comment was auto-generated by Graphite.

github-actions · 2023-09-27T17:49:01Z

Deploy preview for dagster-university ready!

✅ Preview
https://dagster-university-q2tdyhick-elementl.vercel.app
https://pdrm-dagster-elt.dagster-university.dagster-docs.io

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

github-actions · 2023-09-27T17:51:39Z

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-axpkygfg3-elementl.vercel.app
https://pdrm-dagster-elt.components-storybook.dagster-docs.io

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

github-actions · 2023-09-27T17:54:36Z

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-3vkrfmi38-elementl.vercel.app
https://pdrm-dagster-elt.core-storybook.dagster-docs.io

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

python_modules/libraries/dagster-embedded-elt/dagster_embbeded_elt/sling/asset_defs.py

python_modules/libraries/dagster-embedded-elt/dagster_embedded_elt/sling/asset_defs.py

python_modules/libraries/dagster-embedded-elt/dagster_embedded_elt/sling/resources.py

python_modules/libraries/dagster-embedded-elt/dagster_embedded_elt/sling/types.py

github-actions · 2023-09-29T03:37:35Z

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-ot6tliece-elementl.vercel.app
https://pdrm-dagster-elt.dagster.dagster-docs.io

Direct link to changed pages:

docs/content/integrations/embedded-elt.mdx

petehunt

i am not appropriate to be the sole accept, but the api design and docs lgtm, barring a few small notes

python_modules/libraries/dagster-embedded-elt/dagster_embedded_elt_tests/test.csv

python_modules/libraries/dagster-embedded-elt/dagster_embedded_elt/sling/asset_defs.py

Co-authored-by: Ben Pankow <[email protected]>

schrockn · 2023-10-03T15:30:49Z

python_modules/libraries/dagster-embedded-elt/dagster_embedded_elt/sling/asset_defs.py

+        update_key = [update_key]
+
+    @multi_asset(
+        compute_kind="sling", specs=[asset_spec], required_resource_keys={sling_resource_key}


schrockn

This looks nice and well-document. A few comments:

This is a very large PR. We generally like to work in more incremental, digestible pieces. This leads to higher quality, more efficient code review and better outcomes.
There are some complicated components (esp regexs) that are worthy on independent unit tests, so I'm little concerned about test coverage here.

However:

Take my comments as things to note for future and followups.

schrockn · 2023-10-03T15:37:16Z

python_modules/libraries/dagster-embedded-elt/dagster_embedded_elt/sling/resources.py

+
+    @staticmethod
+    def _exec_sling_cmd(cmd, stdin=None, stdout=PIPE, stderr=STDOUT) -> Generator[str, None, None]:
+        ansi_escape = re.compile(r"\x1B(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])")


this regex is complex enough that it is worthy of it's own comment and independent unit tests

schrockn · 2023-10-03T15:37:43Z

python_modules/libraries/dagster-embedded-elt/dagster_embedded_elt/sling/resources.py

+            assert proc.stdout
+
+            for line in proc.stdout:
+                fmt_line = str(line, "utf-8")


is this another way of going line.decode("utf-8")?

schrockn · 2023-10-03T15:38:09Z

python_modules/libraries/dagster-embedded-elt/dagster_embedded_elt/sling/resources.py

+    def _exec_sling_cmd(cmd, stdin=None, stdout=PIPE, stderr=STDOUT) -> Generator[str, None, None]:
+        ansi_escape = re.compile(r"\x1B(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])")
+        with Popen(cmd, shell=True, stdin=stdin, stdout=stdout, stderr=stderr) as proc:
+            assert proc.stdout
+
+            for line in proc.stdout:
+                fmt_line = str(line, "utf-8")
+                clean_line = ansi_escape.sub("", fmt_line).replace("INF", "")
+                yield clean_line
+
+            proc.wait()
+            if proc.returncode != 0:
+                raise Exception("Sling command failed with error code %s", proc.returncode)


this whole code path def a bit scary and I wish it had its own test suite

schrockn · 2023-10-03T15:38:36Z

python_modules/libraries/dagster-embedded-elt/dagster_embedded_elt/sling/resources.py

+        if self.source_connection.type == "file" and not source_stream.startswith("file://"):
+            source_stream = "file://" + source_stream
+
+        if self.target_connection.type == "file" and not target_object.startswith("file://"):
+            target_object = "file://" + target_object


This seems both very magical and very undocumented

schrockn · 2023-10-03T15:40:45Z

python_modules/libraries/dagster-embedded-elt/dagster_embedded_elt/sling/asset_defs.py

+        primary_key (Optional[Union[str, List[str]]], optional): The optional primary key to use when syncing.
+        update_key (Optional[Union[str, List[str]]], optional): The optional update key to use when syncing.


It would be nice to clarify that this is not an asset key. Threw me for a second.

schrockn · 2023-10-03T15:42:37Z

python_modules/libraries/dagster-embedded-elt/dagster_embedded_elt/sling/asset_defs.py

+        compute_kind="sling", specs=[asset_spec], required_resource_keys={sling_resource_key}
+    )
+    def sync(context: AssetExecutionContext) -> MaterializeResult:
+        sling: SlingResource = getattr(context.resources, sling_resource_key)


we should really have a better API for this where you can just get a dict of resources

schrockn · 2023-10-03T15:43:39Z

python_modules/libraries/dagster-embedded-elt/dagster_embedded_elt/sling/asset_defs.py

+            match = re.search(r"(\d+) rows", stdout_line)
+            if match:
+                last_row_count_observed = int(match.group(1))
+            context.log.info(stdout_line)


info or debug?

## Summary & Motivation This adds the `dagster-elt` integration which aims to be a set of embedded ELT/ETL tooling to help data engineers with common ingestion/transform tasks such as ingesting data to and from databases, warehouses, and files. This first iteration is a wrapper around the [sling](https://slingdata.io) package. Sling is a lightweight CLI tool that allows users to sync data from a source/destination without the extra burden of heavyweight tooling that other solutions require. ## How I Tested These Changes Unit tests, local testing. --------- Co-authored-by: Ben Pankow <[email protected]>

PedramNavid requested review from benpankow and rexledesma September 21, 2023 04:03

petehunt reviewed Sep 21, 2023

View reviewed changes

python_modules/libraries/dagster-elt/dagster_elt/asset_defs.py Outdated Show resolved Hide resolved

PedramNavid commented Sep 21, 2023

View reviewed changes

python_modules/libraries/dagster-elt/README.md Outdated Show resolved Hide resolved

python_modules/libraries/dagster-elt/dagster_elt/__init__.py Outdated Show resolved Hide resolved

python_modules/libraries/dagster-elt/dagster_elt/asset_defs.py Outdated Show resolved Hide resolved

schrockn requested changes Sep 22, 2023

View reviewed changes

sryza reviewed Sep 22, 2023

View reviewed changes

PedramNavid force-pushed the pdrm/dagster-elt branch from 3489eab to 2b9c00d Compare September 27, 2023 17:46

PedramNavid commented Sep 27, 2023

View reviewed changes

python_modules/libraries/dagster-embedded-elt/dagster_embbeded_elt/sling/asset_defs.py Outdated Show resolved Hide resolved

PedramNavid force-pushed the pdrm/dagster-elt branch from 17f3570 to 9e5a907 Compare September 28, 2023 01:53

benpankow reviewed Sep 28, 2023

View reviewed changes

PedramNavid marked this pull request as ready for review September 29, 2023 03:29

PedramNavid requested a review from erinkcochran87 as a code owner September 29, 2023 03:29

PedramNavid force-pushed the pdrm/dagster-elt branch from 9ede5ec to 8905ad7 Compare September 29, 2023 03:33

PedramNavid force-pushed the pdrm/dagster-elt branch 3 times, most recently from 5fd7cc2 to 88977bb Compare September 29, 2023 04:06

PedramNavid requested a review from schrockn September 29, 2023 04:13

petehunt reviewed Sep 29, 2023

View reviewed changes

docs/content/integrations/embedded-elt.mdx Outdated Show resolved Hide resolved

petehunt reviewed Sep 29, 2023

View reviewed changes

docs/content/integrations/embedded-elt.mdx Outdated Show resolved Hide resolved

petehunt approved these changes Sep 29, 2023

View reviewed changes

petehunt reviewed Sep 29, 2023

View reviewed changes

python_modules/libraries/dagster-embedded-elt/dagster_embedded_elt_tests/test.csv Outdated Show resolved Hide resolved

flarco reviewed Sep 29, 2023

View reviewed changes

python_modules/libraries/dagster-embedded-elt/dagster_embedded_elt/sling/asset_defs.py Outdated Show resolved Hide resolved

PedramNavid and others added 24 commits October 2, 2023 22:08

Add Dagster-elt project

b4d4a28

add asset defs and tests

a8316cc

remove op and rework api

beedf16

simplify API and move to own folder

f7b41cb

fix asset defs

d3bb7b9

rename to dagster-embedded-elt

bd1336d

rename module and use asset spec instead

f21bd6f

Use Materialize Result instead of context

8b90cb0

lint

454259a

placeholder docs

f0ff124

Apply suggestions from code review

619fcce

Co-authored-by: Ben Pankow <[email protected]>

Add docs to build_sling_asset

6f736ae

add docs

5cc2537

add dagster-embedded-elt to pyright and scripts

3223530

Add experimental decorator

44bd19c

linting

a204ba2

fix tox

8f7fddb

address PR comments

32dc474

update imports

85a7026

fix pyright

2ab233c

really fixed this time

27589d9

address pr comments

942685a

update api docs

3af8d85

update api docs

c74c399

PedramNavid force-pushed the pdrm/dagster-elt branch from 2ce0f6e to c74c399 Compare October 3, 2023 05:10

schrockn reviewed Oct 3, 2023

View reviewed changes

schrockn approved these changes Oct 3, 2023

View reviewed changes

PedramNavid merged commit 5f07c5a into master Oct 3, 2023

PedramNavid deleted the pdrm/dagster-elt branch October 3, 2023 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dagster-elt #16681

Add dagster-elt #16681

PedramNavid commented Sep 21, 2023 •

edited

Loading

petehunt commented Sep 22, 2023

schrockn commented Sep 22, 2023

schrockn left a comment

petehunt commented Sep 25, 2023

PedramNavid commented Sep 27, 2023

github-actions bot commented Sep 27, 2023

github-actions bot commented Sep 27, 2023 •

edited

Loading

github-actions bot commented Sep 27, 2023 •

edited

Loading

github-actions bot commented Sep 29, 2023 •

edited

Loading

petehunt left a comment

schrockn Oct 3, 2023

schrockn left a comment •

edited

Loading

schrockn Oct 3, 2023

schrockn Oct 3, 2023

schrockn Oct 3, 2023

schrockn Oct 3, 2023

schrockn Oct 3, 2023

schrockn Oct 3, 2023

schrockn Oct 3, 2023

		primary_key (Optional[Union[str, List[str]]], optional): The optional primary key to use when syncing.
		update_key (Optional[Union[str, List[str]]], optional): The optional update key to use when syncing.

Add dagster-elt #16681

Add dagster-elt #16681

Conversation

PedramNavid commented Sep 21, 2023 • edited Loading

Summary & Motivation

How I Tested These Changes

petehunt commented Sep 22, 2023

schrockn commented Sep 22, 2023

schrockn left a comment

Choose a reason for hiding this comment

petehunt commented Sep 25, 2023

PedramNavid commented Sep 27, 2023

github-actions bot commented Sep 27, 2023

github-actions bot commented Sep 27, 2023 • edited Loading

github-actions bot commented Sep 27, 2023 • edited Loading

github-actions bot commented Sep 29, 2023 • edited Loading

petehunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schrockn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PedramNavid commented Sep 21, 2023 •

edited

Loading

github-actions bot commented Sep 27, 2023 •

edited

Loading

github-actions bot commented Sep 27, 2023 •

edited

Loading

github-actions bot commented Sep 29, 2023 •

edited

Loading

schrockn left a comment •

edited

Loading