Page MenuHomeElementl

[Depends on D7670] Allow solid to wrap fxn without context arg
ClosedPublic

Authored by cdecarolis on Apr 29 2021, 11:13 PM.

Details

Summary

A big point of discussion in the solid invocation discussions has been what to do with solids that don't require a context. Ultimately, we realized that a solution might be to permit solids to not have a context arg if it is not being used. This means less magic for a user to define their solid, and be able to invoke it without a context.

This diff makes it possible for the functions that solids wrap to have no context argument. This ideally should only work when no config schema is provided, and there are no required resource keys.

Test Plan

Migrating over conspicuous unit tests, docs changes, and error message changes. Pictures of docs changes:

Screen Shot 2021-05-05 at 12.59.11 PM.png (1×1 px, 115 KB)

Screen Shot 2021-05-05 at 12.59.23 PM.png (277×915 px, 47 KB)

Screen Shot 2021-05-05 at 12.59.42 PM.png (106×874 px, 20 KB)

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Include config schema in check if context is required

I think it would be useful to add a block of tests that clearly communicate the new setup by having the different permutations all next to each other

python_modules/dagster/dagster/core/decorator_utils.py
29

whats up with the bool? its always True it from what i can tell

python_modules/dagster/dagster/core/decorator_utils.py
29

it's not necessarily true for the solid case (see [*]) and won't necessarily be true if we move forward with allowing other user fxns to exclude the context param. I made it a tuple instead of a single flag arg to future proof having multiple required args, and allowing/disallowing differently, but maybe that is premature optimization

python_modules/dagster/dagster/core/definitions/decorators/solid.py
347

*

I think this is potentially an exciting improvement (despite some initial hesitancy about the magic) and not just for testing. There are other benefits as well:

  1. We could -- in combination with a context-less logging story -- defer the introduction of context until later in the tutorial.
  2. Pandas and Spark solids could be "pure" if the only resource in play is the i/o manager. This would be a *very* smooth dev-ex where you write pure df-to-df transforms
  3. We could kill @lambda_solid as it is totally pointless with this change.

Per Alex's feedback, I think we need a battery of tests to prove the behavior with all variants of all magic names (_, _context, context, etc) and if nothing else to document it.

I would also like to see a battery of tests that are from the standpoint of "I am going to prove this magic is bad by throwing weird stuff at this and getting counterintuitive error messages". Imagine a grumpy engineer setting out to prove this is a bad idea and throwing use cases at it. Putting "context" in some other slot. Required kwargs. That sort of thing. I'm sure there is a lot I'm not thinking of. In general, opening up this type of magic exposes code paths that yield counterintuitive errors. I'd like to see any of those proactively exposed if possible.

to your queue for test suite

This revision now requires changes to proceed.Apr 30 2021, 4:26 PM

Respin after input arg validation refactor

Adding battery of tests

Test battery, fix inference

I think this is pretty awesome and am in favor of merging this.

python_modules/dagster/dagster_tests/core_tests/test_solid_arguments.py
50

this is rough, but predates this diff

88

also very rough.

Think I'm gonna add a follow up / breaking change diff s.t. we're a bit more strict about name validation, ie if context doesn't work, neither should context_ or _context.

_context and context_ are such a minor corner case that im not worried - I think this is basically good to go

this diff should include a documentation and error message pass around how we communicate the rules since they are changing

to your queue for docs/error message pass

ill probably accept it after that, maybe after having @schrockn peak at the test battery since he wanted to see it

This revision now requires changes to proceed.May 4 2021, 8:09 PM

I'll let @alangenfeld accept, but I'm very supportive of this change and excited to see it land!

Added a "context arg provided" attribute to solid definition. We will use this in invocation to determine whether or not a context arg is provided, rather than magically going based on requirements.

revert most recent change and stack into invocation diff

This revision is now accepted and ready to land.May 5 2021, 10:12 PM