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

Change dask executor config depending on if using job #5848

Merged
merged 2 commits into from
Dec 9, 2021

Conversation

dpeng817
Copy link
Contributor

@dpeng817 dpeng817 commented Dec 1, 2021

The check that I chose here is maybe not the correct one. If someone has a pipeline with a mode default, and put the dask executor on it, then we might run into a problem.

@vercel
Copy link

vercel bot commented Dec 1, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/elementl/dagster/GLSke3a6kLkB8RgcHeC8mSzR76wj
✅ Preview: Canceled

[Deployment for 4d3a1ae canceled]

Copy link
Member

@alangenfeld alangenfeld left a comment

Choose a reason for hiding this comment

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

plan context has IPipeline which has the definition, could use that?

also need a test

@dpeng817
Copy link
Contributor Author

dpeng817 commented Dec 1, 2021

plan context has IPipeline which has the definition, could use that?

also need a test

Right, but are we sure we're okay with that? Could the process where this code runs potentially be a place where we aren't okay with loading the definition?

@alangenfeld
Copy link
Member

Could the process where this code runs potentially be a place where we aren't okay with loading the definition?

the host only code paths are separated - especially from this dask executor specific code. I think its fine.

@dpeng817 dpeng817 merged commit 85b8029 into master Dec 9, 2021
@dpeng817 dpeng817 deleted the dask_executor_fix branch December 9, 2021 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants