Page MenuHomePhabricator

Minor refactor of execute.py
ClosedPublic

Authored by jordanbramble on Apr 1 2020, 11:26 PM.

Details

Summary

This is a first stab at addressing shrockn's comments in D2286. I have not yet decided what to about the
call to all((isinstance(x, ... yet. I am putting this up now so we can discuss how to best add test
coverage to ensure these refactors are actually being tested.

Test Plan

pytest python_modules/dagster/dagster_tests/core_tests/execution_tests

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

jordanbramble created this revision.Apr 1 2020, 11:26 PM
alangenfeld added inline comments.Apr 2 2020, 9:22 PM
python_modules/dagster/dagster/core/execution/plan/execute.py
148–152

a test case like

@lambda_solid
def x():
  return 'x'

@lambda_solid
def y():
  return 'x'


@lambda_solid
def no():
  return None

@lambda_solid(input_defs=[InputDefinition('stuff', Optional[List[str]])])
def gather(stuff):
  return 'x'

@pipeline
def pipe():
  gather([x(), y()])
  gather(no())

execute_pipeline(pipe)

should trigger this behavior

jordanbramble added inline comments.Apr 2 2020, 9:59 PM
python_modules/dagster/dagster/core/execution/plan/execute.py
148–152

I've got pipelines running locally that trigger both behaviors that we are touching here. If I wanted to turn the pipeline definition into an e2e test that pytest runs where would I create that?

Another option would be to unit test dagster_event_sequence_for_step() which is the lowest level importable function that traces to _input_values_from_intermediates_manager()

jordanbramble added inline comments.Apr 2 2020, 10:04 PM
python_modules/dagster/dagster/core/execution/plan/execute.py
148–152

I invoking the else case (from config) using:

environment_dict = {'solids': {'x': {'inputs': {'string_input': {'value': 'think'}}}}}


@solid(
    config={
        'list_of_stuff': Field(
            [str],
            default_value=["Dagster", "is", "Great"],
            is_required=False,
            description=("Write some stuff"),
        )
    }
)
def x(context, string_input):
    res = "I {} {} {} really {}!".format(string_input, *context.solid_config['list_of_stuff'])
    context.log.info(res)
    return res


@pipeline
def pipe():
    x()


execute_pipeline(pipe, environment_dict=environment_dict)
alangenfeld requested changes to this revision.Apr 2 2020, 10:17 PM
alangenfeld added inline comments.
python_modules/dagster/dagster/core/execution/plan/execute.py
148–152

If I wanted to turn the pipeline definition into an e2e test that pytest runs where would I create that?

put it somewhere in python_modules/dagster/dagster_tests/core_tests/execution_tests, you can look at the tests already there to get a sense of how to structure it

This revision now requires changes to proceed.Apr 2 2020, 10:17 PM

Adds a test that checks label of TypeCheckData and output for refactor

jordanbramble retitled this revision from First commit refactoring execute.py to Minor refactor of execute.py.Apr 3 2020, 4:35 AM
jordanbramble edited the test plan for this revision. (Show Details)
jordanbramble added inline comments.Apr 3 2020, 4:40 AM
python_modules/dagster/dagster/core/execution/plan/execute.py
148–152

I've added two tests in execution_tests that construct a simple pipeline that does invoke the changed code. It then does the following:

  1. Checks that TypeCheckData.label is as expected, after retrieving the appropriate solid's input_event_dict.
  2. Checks the output is as expected.

I now realize that we actually may already have had test coverage for this, but if not these definitely invoke the changed code.

Adds a test that checks label of TypeCheckData and output for refactor

Adds a test that checks label of TypeCheckData and output for refactor

This revision is now accepted and ready to land.Apr 3 2020, 3:59 PM

rebases on top of D2339

push to staging

This revision was automatically updated to reflect the committed changes.