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

[DOC-430] Add Managing Concurrency Guide #24296

Conversation

cnolanminich
Copy link
Contributor

Adapted / edited the original doc for the new guide format.

I had a couple of questions as I was working on it:

  • Should we keep the following sections or move them to a "concepts" page?
    • glossary
    • Setting priority for ops/assets
    • Freeing concurrency slots
    • Throttling concurrency-limited runs
    • Troubleshooting
  • tag_concurrency_limits could use a major look -- is bug noted here accurate, or does this still work with Postgres / MySQL?
  • question: include Dagster insance settings (dagster.yaml / Dagster+ Deployment settings) examples as inline YAML, keep the folder with the dagster.yaml? To repro, need to export DAGSTER_HOME=$(pwd)/global_concurrency` (and for tag_concurrency, need an OSS version using Postgres / MYSQL)
  • (from Linear ticket) How can I set concurrency limits on a resource? -- I don't think this is possible, other than via using the concurrency tags.

@PedramNavid
Copy link
Contributor

@cnolanminich I noticed this is a draft, would this be ready for review you think?

@cnolanminich
Copy link
Contributor Author

@PedramNavid yes I think so -- I wrote this during the week after docathon and wasn't sure if I should mark it ready for review or as a draft until some of the questions in my comment were addressed. Will mark it ready for review!

@cnolanminich cnolanminich marked this pull request as ready for review September 27, 2024 19:25
@graphite-app graphite-app bot added the area: docs Related to documentation in general label Sep 27, 2024
Copy link
Contributor

I think you will need to tag a reviewer to help get this over the finish line as well.
As to these questions:

Should we keep the following sections or move them to a "concepts" page?

glossary

  • i don't see this in the original or old docs, prefer not to have one.

Setting priority for ops/assets
Freeing concurrency slots
Throttling concurrency-limited runs

  • i think the above 3 can be part of a reference of configurations

Troubleshooting

  • Can be included here as well yes.

Copy link

graphite-app bot commented Sep 28, 2024

Graphite Automations

"Label and add CE on all Docs" took an action on this PR • (09/28/24)

3 reviewers were added to this PR based on Pedram Navid's automation.

@cnolanminich
Copy link
Contributor Author

Setting priority for ops/assets
Freeing concurrency slots
Throttling concurrency-limited runs

i think the above 3 can be part of a reference of configurations

Do you mean added as a reference of configuration to this page, or as a separate page? Will add a troubleshooting section and then tag reviewers, thanks!

@PedramNavid
Copy link
Contributor

Setting priority for ops/assets
Freeing concurrency slots
Throttling concurrency-limited runs

i think the above 3 can be part of a reference of configurations

Do you mean added as a reference of configuration to this page, or as a separate page? Will add a troubleshooting section and then tag reviewers, thanks!

A separate page, fine to link to a [/todo] for now!

@cnolanminich cnolanminich requested a review from prha October 18, 2024 20:15
@cnolanminich
Copy link
Contributor Author

@PedramNavid the deploy docs revamp action is failing, it had succeeded previously and I ran yarn build successfully locally -- anything I might be missing on why the build isn't working?

@prha it looks like you were the original author of the limiting concurrency docs page -- would you be able to review this from a content perspective or recommend someone who can?

@cmpadden
Copy link
Contributor

@PedramNavid the deploy docs revamp action is failing, it had succeeded previously and I ran yarn build successfully locally -- anything I might be missing on why the build isn't working?

@prha it looks like you were the original author of the limiting concurrency docs page -- would you be able to review this from a content perspective or recommend someone who can?

Here's the full error message, @cnolanminich:

[ERROR] Error: Unable to build website for locale en.
    at tryToBuildLocale (/vercel/path0/docs/docs-beta/node_modules/@docusaurus/core/lib/commands/build.js:54:19)
    at async /vercel/path0/docs/docs-beta/node_modules/@docusaurus/core/lib/commands/build.js:65:9
    at async mapAsyncSequential (/vercel/path0/docs/docs-beta/node_modules/@docusaurus/utils/lib/jsUtils.js:21:24)
    at async Command.build (/vercel/path0/docs/docs-beta/node_modules/@docusaurus/core/lib/commands/build.js:63:5) {
  [cause]: Error: Docusaurus found broken links!
  
  Please check the pages of your site in the list below, and make sure you don't reference any path that does not exist.
  Note: it's possible to ignore broken links with the 'onBrokenLinks' Docusaurus configuration, and let the build pass.
  
  Exhaustive list of all broken links found:
  - Broken link on source page path = /guides/tbd/managing-concurrency:
     -> linking to /guides/tbd/running-local-ui-development

@cnolanminich
Copy link
Contributor Author

@PedramNavid the deploy docs revamp action is failing, it had succeeded previously and I ran yarn build successfully locally -- anything I might be missing on why the build isn't working?
@prha it looks like you were the original author of the limiting concurrency docs page -- would you be able to review this from a content perspective or recommend someone who can?

Here's the full error message, @cnolanminich:

[ERROR] Error: Unable to build website for locale en.
    at tryToBuildLocale (/vercel/path0/docs/docs-beta/node_modules/@docusaurus/core/lib/commands/build.js:54:19)
    at async /vercel/path0/docs/docs-beta/node_modules/@docusaurus/core/lib/commands/build.js:65:9
    at async mapAsyncSequential (/vercel/path0/docs/docs-beta/node_modules/@docusaurus/utils/lib/jsUtils.js:21:24)
    at async Command.build (/vercel/path0/docs/docs-beta/node_modules/@docusaurus/core/lib/commands/build.js:63:5) {
  [cause]: Error: Docusaurus found broken links!
  
  Please check the pages of your site in the list below, and make sure you don't reference any path that does not exist.
  Note: it's possible to ignore broken links with the 'onBrokenLinks' Docusaurus configuration, and let the build pass.
  
  Exhaustive list of all broken links found:
  - Broken link on source page path = /guides/tbd/managing-concurrency:
     -> linking to /guides/tbd/running-local-ui-development

Thanks! Fixed the link and it built successfully 🎉

<CodeExample filePath="guides/tbd/concurrency-tag-key-asset.py" language="python" title="No more than 1 asset running with a tag of 'database'" />

</TabItem>
<TabItem value="Op Tag" label="Asset tag concurrency limits">
Copy link
Member

Choose a reason for hiding this comment

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

op tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed the label


<Tabs>
<TabItem value="Asset Tag" label="Asset tag concurrency limits">
<CodeExample filePath="guides/tbd/concurrency-tag-key-asset.py" language="python" title="No more than 1 asset running with a tag of 'database'" />
Copy link
Member

Choose a reason for hiding this comment

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

maybe too wordy, but consider adding "across all runs"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's more wordy but I think precision is good for this since it's a complex topic


</TabItem>
<TabItem value="Op Tag" label="Asset tag concurrency limits">
<CodeExample filePath="guides/tbd/concurrency-tag-key-op.py" language="python" title="No more than 1 op running with a tag of 'database'" />
Copy link
Member

Choose a reason for hiding this comment

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

ditto ("across all runs")

Or you can configure it in the job definition:

<Tabs>
<TabItem value="Asset Tag with Job" label="Asset tag concurrency limits in a job">
Copy link
Member

Choose a reason for hiding this comment

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

in a job is a little bit misleading... It's really within a run.

e.g. I can kick off two runs of job foo, and each run will have 1 "database" tag asset running.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that was sloppy, corrected!

</TabItem>
</Tabs>

Or you can configure it in the job definition:
Copy link
Member

Choose a reason for hiding this comment

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

It might be helpful to explain why you might use one versus the other.

For example... if you have some resource-constraints on something like BigQuery, you might want to configure global concurrency limits so that you don't tax this resource which is used across runs.

The run-scoped tag concurrency limits are useful when limiting compute resources within the run, if it's using something like the multiprocess executor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, reworded / added some context on when you might want to do this


Need screenshot here

## Prevent runs from starting if another run is already occurring (advanced)
Copy link
Member

Choose a reason for hiding this comment

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

should we talk about block_op_concurrency_limited_runs at all? There's a section here about it in the old docs: https://docs.dagster.io/guides/limiting-concurrency-in-data-pipelines#throttling-concurrency-limited-runs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pedram and I decided to punt that and a few other items to a "reference" page to be written

Copy link
Contributor Author

@cnolanminich cnolanminich left a comment

Choose a reason for hiding this comment

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

thanks @prha! updated per your feedback

Or you can configure it in the job definition:

<Tabs>
<TabItem value="Asset Tag with Job" label="Asset tag concurrency limits in a job">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that was sloppy, corrected!

</TabItem>
</Tabs>

Or you can configure it in the job definition:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, reworded / added some context on when you might want to do this

<CodeExample filePath="guides/tbd/concurrency-tag-key-asset.py" language="python" title="No more than 1 asset running with a tag of 'database'" />

</TabItem>
<TabItem value="Op Tag" label="Asset tag concurrency limits">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed the label


<Tabs>
<TabItem value="Asset Tag" label="Asset tag concurrency limits">
<CodeExample filePath="guides/tbd/concurrency-tag-key-asset.py" language="python" title="No more than 1 asset running with a tag of 'database'" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's more wordy but I think precision is good for this since it's a complex topic


Need screenshot here

## Prevent runs from starting if another run is already occurring (advanced)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pedram and I decided to punt that and a few other items to a "reference" page to be written

Copy link
Contributor

@cmpadden cmpadden 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 that this guide has come a long way, and I'd love to get this merged. I've been sharing the PR to this to some community members, and would love to get this on docs-preview.dagster.io.

We can definitely iterate more, and will likely do another pass of everything again.

@cnolanminich
Copy link
Contributor Author

Per @cmpadden, the oss-internal-compat buildkite failure is flaky and is ok to merge

@cnolanminich cnolanminich merged commit 910f5c5 into master Oct 29, 2024
1 of 3 checks passed
@cnolanminich cnolanminich deleted the christian/doc-430-write-managing-concurrency-in-your-pipelines branch October 29, 2024 19:08
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