Page MenuHomePhabricator

improve reconstructable()
ClosedPublic

Authored by alangenfeld on Tue, May 12, 8:48 PM.

Details

Summary

Handle decorators and imrpove errors.

Test Plan

new test set

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

alangenfeld created this revision.Tue, May 12, 8:48 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Tue, May 12, 9:01 PM
Harbormaster failed remote builds in B11208: Diff 13773!

split 2/3 difference

alangenfeld requested review of this revision.Wed, May 13, 6:58 PM
max accepted this revision.Wed, May 13, 7:15 PM

still think it would be nice if @pipeline gave you this for free

This revision is now accepted and ready to land.Wed, May 13, 7:15 PM

still think it would be nice if @pipeline gave you this for free

you have a model for how would that work? how bout cases like [2]?

The paths i thought of don't have consistent behavior which led me away from them.

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_reconstructable.py
117–120

[2]

schrockn requested changes to this revision.Wed, May 13, 8:55 PM

i think it would be good to extract those checks into functions in seven and test them

python_modules/dagster/dagster/core/definitions/reconstructable.py
186–207

seems like these should live in seven. i imagine they will get more complicated

227–239

generally prefer look before you leap but nbd

This revision now requires changes to proceed.Wed, May 13, 8:55 PM
alangenfeld added inline comments.Thu, May 14, 2:51 AM
python_modules/dagster/dagster/core/definitions/reconstructable.py
227–239

I started down that path but theres a lot of cases to check (and py2/3 compat sucks a lot) and those still don't solve for the cases like someone declaring a function as **kwargs and expecting certain args to be present.

alangenfeld planned changes to this revision.Thu, May 14, 4:21 PM

Macro khan:  PYTHON 2

schrockn requested changes to this revision.Thu, May 14, 4:26 PM
schrockn added inline comments.
python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_reconstructable.py
81–118

whats the user experience in py2 then?

This revision now requires changes to proceed.Thu, May 14, 4:26 PM
alangenfeld added inline comments.Thu, May 14, 4:27 PM
python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_reconstructable.py
81–118

they would see

"pipe" not found in file <full path>

alangenfeld requested review of this revision.Thu, May 14, 4:29 PM

which i think is sufficient given python 2 is deprecated

This revision is now accepted and ready to land.Thu, May 14, 4:30 PM

I can update the CodePointer error to mention module scope in some way

This revision was automatically updated to reflect the committed changes.