Page MenuHomePhabricator

TODO: Split into two diffs
AbandonedPublic

Authored by max on Wed, Sep 11, 6:13 PM.

Details

Reviewers
None
Summary
  • Adds a test for the hammer pipeline, running on dask, through the dagit codepath.
  • Allows users to pass an instance to the make_airflow_dag constructor -- events returned
Test Plan

Unit

Diff Detail

Repository
R1 dagster
Branch
dask-events-3
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

max created this revision.Wed, Sep 11, 6:13 PM
max edited the summary of this revision. (Show Details)Wed, Sep 11, 6:42 PM
alangenfeld added inline comments.Wed, Sep 11, 6:46 PM
python_modules/dagster-dask/dagster_dask/engine.py
33
python_modules/dagster-graphql/dagster_graphql/client/mutations.py
37–40

i think its probably better to enforce it be passed as opposed to defaulting to ephemeral

python_modules/dagster-graphql/dagster_graphql/implementation/execution.py
248

thanks

alangenfeld requested changes to this revision.Wed, Sep 11, 6:52 PM

to your queue

python_modules/dagster/dagster/core/instance/__init__.py
191–194

working on this

This revision now requires changes to proceed.Wed, Sep 11, 6:52 PM
Harbormaster failed remote builds in B3701: Diff 4611!
max updated this revision to Diff 4622.Wed, Sep 11, 9:03 PM

es Up

max marked an inline comment as done.Wed, Sep 11, 9:06 PM
max added inline comments.
python_modules/dagster-dask/dagster_dask/engine.py
33

ok

python_modules/dagster-graphql/dagster_graphql/client/mutations.py
37–40

I would like to see this client eventually be user-facing (like, for people who want to noodle around with GraphQL but not do all the parsing and error handling and query construction themselves), so I'm not sure about this.

max updated this revision to Diff 4623.Wed, Sep 11, 9:08 PM
max marked an inline comment as done.

Fix

max updated this revision to Diff 4624.Wed, Sep 11, 9:14 PM

Black

schrockn requested changes to this revision.Wed, Sep 11, 10:31 PM
schrockn added a subscriber: schrockn.

why are we doing airflow changes in a diff titled "Fixes for Dask execution"?

python_modules/dagster-airflow/dagster_airflow/factory.py
63

i think we should do

instance = check.inst_param(instance) if instance else Dagster.get(...)

because that function does some real work

python_modules/dagster-airflow/dagster_airflow/operators/docker_operator.py
147–150

i still feel icky when i see business logic against the environment dict

This revision now requires changes to proceed.Wed, Sep 11, 10:31 PM
max updated this revision to Diff 4648.Thu, Sep 12, 1:52 AM

O RLY

max added inline comments.Thu, Sep 12, 1:56 AM
python_modules/dagster-airflow/dagster_airflow/factory.py
63

yep

python_modules/dagster-airflow/dagster_airflow/operators/docker_operator.py
147–150

@alangenfeld and I discussed this -- the alternative is to push knowledge about the Airflow executor deeper into the core. we don't have a good abstraction rn to express semantic limitations/predicates on elements of the config dict beyond their types, so engines, etc., can't declare "I need these restrictions above and beyond typechecking". if we don't perform these checks / substitutions here, then perfectly valid config (which works fine against the in-process executor) will appear fine against airflow, but then lead to opaque failures in the middle of the pipeline when intermediates are not available.

max retitled this revision from Fixes for Dask execution to Fixes for Dask and Airflow execution with DagsterInstance.Thu, Sep 12, 1:57 AM
max updated this revision to Diff 4649.Thu, Sep 12, 1:57 AM
max retitled this revision from Fixes for Dask and Airflow execution with DagsterInstance to Fixes for Dask execution.

Nits

max edited the summary of this revision. (Show Details)Thu, Sep 12, 1:58 AM
max retitled this revision from Fixes for Dask execution to Fixes for Dask and Airflow execution with DagsterInstance.
max retitled this revision from Fixes for Dask and Airflow execution with DagsterInstance to TODO: Split into two diffs.Thu, Sep 12, 2:00 AM
max edited the summary of this revision. (Show Details)
max removed reviewers: Restricted Project, alangenfeld, schrockn.
max abandoned this revision.Thu, Sep 12, 2:26 AM

Abandoning in favor of D1019 and D1021