Page MenuHomePhabricator

Rename get_or_create_run() => register_managed_run() and other clean up
ClosedPublic

Authored by catherinewu on Tue, May 19, 10:18 PM.

Details

Summary

Follow up to address comments in https://dagster.phacility.com/D2719
From @schrockn "I also think we should do some things to the naming to make this clearer (feel free to do this in follow on)

Make it clear that this is the codepath for creating "MANAGED" runs. register_managed_run maybe?
Eliminate the status argument.
While we're at it, eliminate all default values for the arguments. It is super easy to forget one and they cause more harm than their potential convenience."

Test Plan

No change in functionality (except one change in flyte compiler)

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

catherinewu created this revision.Tue, May 19, 10:18 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Tue, May 19, 10:30 PM
Harbormaster failed remote builds in B11597: Diff 14258!
catherinewu requested review of this revision.Tue, May 19, 10:46 PM

remove default values for create_run()

catherinewu retitled this revision from get_or_create_run() clean up to Rename get_or_create_run() => register_managed_run() and other clean up.Tue, May 19, 11:30 PM
catherinewu edited the summary of this revision. (Show Details)
catherinewu edited the test plan for this revision. (Show Details)
catherinewu added a subscriber: schrockn.
jordanbramble added inline comments.Wed, May 20, 12:54 AM
python_modules/libraries/dagster-flyte/dagster_flyte/flyte_compiler.py
49

This looks good to me.

schrockn requested changes to this revision.Wed, May 20, 1:05 AM

So i *would* suggest adding a utility function wrapper for test cases to avoid boilerplate. my concern is mostly about being explicit for production code paths.

This revision now requires changes to proceed.Wed, May 20, 1:05 AM

add utility function for tests

schrockn requested changes to this revision.Wed, May 20, 12:52 PM

btw: no need to break this up at this point but this is a good example where chopping into 2 diffs (rename and change in a default args) would have been good. rename would be been approved and then other diff could be focused on the "default" discussion

python_modules/dagster/dagster/core/test_utils.py
115–120

I think this is a little too clever.

I think we should just have a function create_test_run or create_run_for_test or something. We'll probably repeat this pattern and sometimes defaults won't be none

python_modules/libraries/dagster-flyte/dagster_flyte/flyte_compiler.py
60–61

good example of why this is a good idea! we should definitely be setting snapshots here. would be good to file a task to keep track

This revision now requires changes to proceed.Wed, May 20, 12:52 PM

remove clever thing

schrockn requested changes to this revision.Wed, May 20, 8:48 PM

being a pill here but I really think we should avoid all cleverness. this makes it so you have to go through a level of indirection to see what args they are. just list out all the args and list the default values right there.

in general, optimize for the code reader later on, rather than the number of characters typed not. this will be read orders of magnitude more times than you write it.

python_modules/dagster/dagster/core/test_utils.py
117–120

generally prefer just listing out args for documentation purposes

python_modules/dagster/dagster/seven/__init__.py
148–151

yeah let's not do this in this diff

python_modules/dagster/dagster/utils/test/__init__.py
237–250

use test helper?

This revision now requires changes to proceed.Wed, May 20, 8:48 PM
schrockn requested changes to this revision.Wed, May 20, 10:24 PM
schrockn added inline comments.
python_modules/dagster/dagster/core/test_utils.py
136–149

register_managed_run_for_test(

pipeline_name=None,
run_id=None,
#... and so forth

)

That way IDEs know about it

python_modules/dagster/dagster/utils/test/__init__.py
237

just test helper?

This revision now requires changes to proceed.Wed, May 20, 10:24 PM
catherinewu added inline comments.Wed, May 20, 10:43 PM
python_modules/dagster/dagster/core/test_utils.py
117–120

sounds good, will do. I was mainly concerned about users having to update this function every time create_run is updated, but that's fine now that create_run_for_test is clear

python_modules/dagster/dagster/utils/test/__init__.py
237–250

this is actually called in check_dagster_type so I thought I'd leave all the args there

catherinewu added inline comments.Thu, May 21, 5:47 PM
python_modules/dagster/dagster/utils/test/__init__.py
237

the outer function yield_empty_pipeline_context() is called in check_dagster_type(), which is part of the public api so i think it falls more under production code than test code?

schrockn accepted this revision.Thu, May 21, 7:03 PM

great thanks

This revision is now accepted and ready to land.Thu, May 21, 7:03 PM