Details

Reviewers

alangenfeld
• sandyryza

Commits

R1:05b903972657: Refactor the validation step for positional args in decorators

Summary

The way we validate required positional arguments in our decorators is hard to understand. This attempts to make the flow a bit easier to follow (and easier to change).

Test Plan

Unit tests

Diff Detail

Repository

R1 dagster

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• cdecarolis created this revision.Apr 29 2021, 9:24 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 29 2021, 9:41 PM

Harbormaster failed remote builds in B29724: Diff 36510!

Slight implementation change in preparation for next change

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 29 2021, 10:01 PM

Harbormaster failed remote builds in B29726: Diff 36512!

Harbormaster completed remote builds in B29733: Diff 36520.Apr 29 2021, 11:11 PM

• cdecarolis requested review of this revision.Apr 29 2021, 11:11 PM

• cdecarolis added inline comments.Apr 29 2021, 11:15 PM

python_modules/dagster/dagster/core/decorator_utils.py
22	lambda is funky but our error messages here arent uniform

• cdecarolis added inline comments.Apr 29 2021, 11:16 PM

python_modules/dagster/dagster/core/definitions/decorators/solid.py
365	should we get rid of this comment? I dont feel like it makes much sense
365–367	This also is misleading because they're still positional parameters, they're just not the context

some ideas inline - I think this can use a second pass. Its a little better but not a clear step up imo

python_modules/dagster/dagster/core/definitions/decorators/hook.py
48–49	its odd how many of these call sites don't use the output on this function also agree the lambda for error message is an unfortunate indirection. Would make it hard to improve the error message with more granular context what if we had a couple more granular functions instead? `get_function_params` `has_expected_positionals` split part probably doesnt need a func

alangenfeld added inline comments.Apr 30 2021, 3:44 PM

python_modules/dagster/dagster/core/definitions/decorators/solid.py
365	ye
365–367	i think its communicating "enforced position" or something maybe but I agree it can be more clear - "input args after the context" or something
372	why this cast? shouldn't the func def get this?