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.

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

[docs][guide] writing a multi-asset backed integration #22910

Merged
merged 29 commits into from
Aug 23, 2024

Conversation

cmpadden
Copy link
Contributor

@cmpadden cmpadden commented Jul 9, 2024

  • Introduces a guide that walks through the process of building a multi-asset decorator with translator

NOTE: Once content has been reviewed, I can refactor the snippets to docs_snippets for proper linting.

Copy link
Contributor Author

cmpadden commented Jul 9, 2024

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

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

@cmpadden cmpadden changed the title [docs][guide] writing a multi-asset backed integration DRAFT: [docs][guide] writing a multi-asset backed integration Jul 9, 2024
@cmpadden cmpadden changed the title DRAFT: [docs][guide] writing a multi-asset backed integration [docs][guide] writing a multi-asset backed integration Jul 12, 2024
@cmpadden cmpadden marked this pull request as ready for review July 12, 2024 17:28
@graphite-app graphite-app bot added the area: docs Related to documentation in general label Jul 12, 2024
@graphite-app graphite-app bot requested a review from erinkcochran87 July 12, 2024 17:28
@cmpadden cmpadden force-pushed the colton/guide-integration-approaches branch from 0533f7b to b286464 Compare July 12, 2024 18:18
@cmpadden cmpadden force-pushed the colton/guide-integration-multi-asset branch from 11e40ac to 07622a6 Compare July 12, 2024 18:19
@cmpadden cmpadden force-pushed the colton/guide-integration-approaches branch from 11fdb33 to 950746f Compare July 26, 2024 13:56
@PedramNavid PedramNavid requested a review from danielgafni July 29, 2024 16:32
Copy link
Contributor

@danielgafni danielgafni left a comment

Choose a reason for hiding this comment

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

I think this content looks really great!

I have 2 thoughts:

  1. Might make sense to add a sentence (or a short code snippet) explicitly explaining that asset dependencies will still work (and why)
  2. We might want to link this blogpost (what a coincidence!) which explains how to go further into more advanced asset customization (like providing common data transformations on library side). The tutorial suggests creating a resource to handle common logic, and this is an alternative approach. It might be slightly more convenient because the user doesn't need to call the resource. I don't know how often is something like this needed, just a suggestion.
  3. Should the tutorial also provide an example of writing an IOManager to handle common IO instead doing it in a Resource? Or perhaps just link to IOManager docs?

@cmpadden cmpadden force-pushed the colton/guide-integration-multi-asset branch from 07622a6 to 8da012a Compare August 7, 2024 18:15
@cmpadden
Copy link
Contributor Author

cmpadden commented Aug 7, 2024

I think this content looks really great!

I have 2 thoughts:

1. Might make sense to add a sentence (or a short code snippet) explicitly explaining that asset dependencies will still work (and why)

2. We might want to link [this](https://dagster.io/blog/unlocking-flexible-pipelines-customizing-asset-decorator) blogpost (what a coincidence!) which explains how to go further into more advanced asset customization (like providing common data transformations on library side). The tutorial suggests creating a resource to handle common logic, and this is an alternative approach. It might be slightly more convenient because the user doesn't need to call the resource. I don't know how often is something like this needed, just a suggestion.

3. Should the tutorial also provide an example of writing an `IOManager` to handle common IO instead doing it in a `Resource`? Or perhaps just link to `IOManager` docs?

Thanks for the feedback, @danielgafni.

I've included a link to your blog post - what a coincidence indeed! And made it more explicit in that dependencies can be defined.

I'm going to hold off on I/O managers for now as we don't do that for the Sling or dlt integration.

Edit: I've also included links in the side menu.

@cmpadden cmpadden force-pushed the colton/guide-integration-approaches branch from 950746f to 6561292 Compare August 8, 2024 17:16
@cmpadden cmpadden force-pushed the colton/guide-integration-multi-asset branch from 49e0912 to 0f9c772 Compare August 8, 2024 17:17
Copy link
Contributor

@erinkcochran87 erinkcochran87 left a comment

Choose a reason for hiding this comment

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

Back to you!

@cmpadden cmpadden requested a review from erinkcochran87 August 9, 2024 19:15
@cmpadden
Copy link
Contributor Author

cmpadden commented Aug 9, 2024

Thanks again for your feedback, @erinkcochran87 , I believe I have addressed it all.

Copy link
Contributor

@yuhan yuhan left a comment

Choose a reason for hiding this comment

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

left comments around IA/navigation. overall looking good - we may be able to simplify the metadata threading, but i think we can land this and iterate.

docs/content/_navigation.json Outdated Show resolved Hide resolved
- An understanding of Python decorators — [Real Python's Primer on Python Decorators](https://realpython.com/primer-on-python-decorators/) is a fantastic introduction

---

Copy link
Contributor

Choose a reason for hiding this comment

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

feel free to push back, and we can address it in the jobs-to-be-done world

i wonder if we could add a "when to model your integration in this way" section, where we can explain the whys (e.g. composability), map pattern to category like "ETL", "BI" and also list example implementations like you did on the "Approaches to writing integrations"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do like that idea - however, let's see if we can merge this first, and then make improvements.

Much of this may be rewritten or scrapped as a part of the docs overhaul. If we can merge this, then we can consider restructuring in the docathon if that works for you.

cmpadden and others added 21 commits August 23, 2024 10:55
@cmpadden cmpadden force-pushed the colton/guide-integration-multi-asset branch from 6a64f16 to 5dde31a Compare August 23, 2024 14:56
Copy link
Contributor Author

Merge activity

  • Aug 23, 2:46 PM EDT: @cmpadden started a stack merge that includes this pull request via Graphite.

@cmpadden cmpadden changed the base branch from colton/guide-integration-approaches to graphite-base/22910 August 23, 2024 18:48
@cmpadden cmpadden changed the base branch from graphite-base/22910 to master August 23, 2024 18:53
@cmpadden cmpadden merged commit 90c6304 into master Aug 23, 2024
2 of 3 checks passed
@cmpadden cmpadden deleted the colton/guide-integration-multi-asset branch August 23, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: docs Related to documentation in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants