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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] - Improve Pipes API docs #21707

Merged
merged 8 commits into from May 10, 2024
Merged

Conversation

erinkcochran87
Copy link
Contributor

@erinkcochran87 erinkcochran87 commented May 7, 2024

Summary & Motivation

This PR adds information to the Pipes and dagster-pipes API references to clarify where and how each set of APIs should be used. Previously, the introductions for these references were vague and confusing - it was difficult to tell which set of APIs you needed and when.

I've also added broken the APIs up into sections, grouped by type, and added information about what each group does. I've also added links to appropriate guides to make cross-referencing content easier.

After:

Preview links: Dagster Pipes reference; dagster-pipes reference

Screenshot 2024-05-10 at 11 37 05 AM
Screenshot 2024-05-10 at 11 37 18 AM

Before:

Screenshot 2024-05-08 at 11 03 29 AM

Screenshot 2024-05-08 at 11 03 44 AM

How I Tested These Changes

馃憖 , local

@graphite-app graphite-app bot added the area: docs Related to documentation in general label May 7, 2024
@erinkcochran87 erinkcochran87 requested a review from sryza May 7, 2024 22:13
@schrockn
Copy link
Member

schrockn commented May 8, 2024

Important to note that:

  1. Writing and using pipes clients
  2. Writing code in the external process that just streams metadata back to Dagster.

They are two different use cases targeted to different personas. The "platform owner" more likely to be working on and setting up clients; the "pipeline builder" will be including dagster-pipes in the external environment. Critical point: when someone includes dagster-pipes it does not include the dagster library.

So while the previous organization wasn't ideal merging all the pipes client stuff (which is much more complex) with dagster-pipes API reference also not ideal.

@schrockn
Copy link
Member

schrockn commented May 8, 2024

Also heads up that I'm fixing pipes docs issues: #21719

@erinkcochran87
Copy link
Contributor Author

@schrockn Heard. Took another swing at this - basically left all the formatting, but moved things back to their separate pages and beefed up the intros. I called out exactly where the APIs in the reference should be used and added callouts for users who may be looking for the other reference.

For example, I added this to the page for dagster-pipes:

...
Looking to set up a Pipes client in Dagster? Refer to the Dagster Pipes API reference.

Note: This library isn't included with dagster and must be installed separately.

Updated dagster-pipes reference:

Screenshot 2024-05-08 at 2 52 26 PM

Updated Dagster Pipes reference:

Screenshot 2024-05-08 at 2 52 45 PM

Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

Looking much better! My final recommendation is that we further subdivide docs into 1) stuff that people need to know if they are consuming an integration versus 2) people building a new integration (much more advanced)


.. currentmodule:: dagster_pipes

The ``dagster-pipes`` library is intended for inclusion in an external process that integrates with Dagster using the `Pipes </concepts/dagster-pipes>`_ protocol. This could be in an environment like Databricks, Kubernetes, or Docker. Using this library, you can write code in the external process that streams metadata back to Dagster.
Copy link
Member

Choose a reason for hiding this comment

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

I think this can also mention that pipes passes parameters to the external process from the orchestration environment and you can access them there. It's a key value prop of the system.

docs/sphinx/sections/api/apidocs/pipes.rst Show resolved Hide resolved
@erinkcochran87 erinkcochran87 dismissed schrockn鈥檚 stale review May 9, 2024 22:51

Re-requesting to get back in queue

@erinkcochran87
Copy link
Contributor Author

@schrockn Made the changes you asked for! Let me know if you have specific wording in mind and I'll make the updates.

Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

Looks great! Please heed final comment. 馃檹馃徎

docs/sphinx/sections/api/apidocs/pipes.rst Outdated Show resolved Hide resolved
@erinkcochran87 erinkcochran87 changed the title [docs] - Pipes API experiment [docs] - Improve Pipes API docs May 10, 2024
@erinkcochran87 erinkcochran87 merged commit b8170b1 into master May 10, 2024
2 checks passed
@erinkcochran87 erinkcochran87 deleted the erin/pipes-api-experiment branch May 10, 2024 15:41
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.

None yet

2 participants