Page MenuHomePhabricator

Surface step skipped in Airflow
ClosedPublic

Authored by natekupp on Jul 26 2019, 7:41 PM.

Details

Reviewers
schrockn
Group Reviewers
Restricted Project
Commits
R1:9cf1616c61a7: Surface step skipped in Airflow
Test Plan

unit

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

natekupp created this revision.Jul 26 2019, 7:41 PM
natekupp added a reviewer: Restricted Project.Jul 26 2019, 8:56 PM

did we figure out when in the airflow rev history this was added?

python_modules/dagster-airflow/dagster_airflow/operators.py
280–284

unrelated to your diff, but I actually think function name is a little misleading. This is actually just executing one step in the normal case or just a few coalesced steps rather than the entire pipeline

schrockn requested changes to this revision.Jul 26 2019, 10:23 PM

Actually one test case to think about. An Airflow operator that executes N steps and the last one skips. Or we should constrain this so that we only execute a single step, which might be the right thing

This revision now requires changes to proceed.Jul 26 2019, 10:23 PM
natekupp updated this revision to Diff 3250.Jul 26 2019, 10:46 PM

get docker version working and add test case

schrockn added inline comments.Jul 26 2019, 10:49 PM
python_modules/dagster-airflow/dagster_airflow/operators.py
279–285

is there anyway we can consolidate the two code paths? why does one have context['task'] and why does one have dag.get_task(task_id)

schrockn added inline comments.Jul 26 2019, 10:52 PM
python_modules/dagster-airflow/dagster_airflow/operators.py
289

just a bit concerned about drift we insert more logic here

schrockn requested changes to this revision.Jul 26 2019, 10:52 PM

popping back in your queue

This revision now requires changes to proceed.Jul 26 2019, 10:52 PM
natekupp added inline comments.Jul 26 2019, 11:05 PM
python_modules/dagster-airflow/dagster_airflow/operators.py
279–285

good call. the underlying implementations are annoyingly a little different. in DockerOperator we're expected to implement execute which gets passed the Airflow context, whereas PythonOperator you're expected to provide a callable, and Airflow calls you with kwargs - fortunately they dump context into kwargs so I can get these at least a little closer in implementation

schrockn accepted this revision.Jul 26 2019, 11:09 PM

kk

please consider final comment

python_modules/dagster-airflow/dagster_airflow/operators.py
337–345

i still think there should be common function here that accepts execution_date and task

This revision is now accepted and ready to land.Jul 26 2019, 11:09 PM
natekupp updated this revision to Diff 3257.Jul 27 2019, 2:42 AM

rebase and comments

natekupp added inline comments.Jul 27 2019, 2:45 AM
python_modules/dagster-airflow/dagster_airflow/operators.py
337–345

I created a DagsterSkipMixin wrapper over SkipMixin to gather this logic in one place, lmk if this is along the lines of what you were thinking here

This revision was automatically updated to reflect the committed changes.