Page MenuHomePhabricator

[lakehouse] accept an optional "context" arg for computations
AbandonedPublic

Authored by sandyryza on Sep 30 2020, 12:35 AM.

Details

Reviewers
yuhan
cdecarolis
Summary

Depends on D4562.

Test Plan

added tests

Diff Detail

Repository
R1 dagster
Branch
lakehouse-context (branched from master)
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

sandyryza retitled this revision from lakehouse context to [lakehouse] accept an optional "context" arg for computations.Sep 30 2020, 12:37 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Sep 30 2020, 12:53 AM
Harbormaster failed remote builds in B18885: Diff 22938!

should we be more restrict to limit context to be a required arg and at the first position like all other decorators do?

should we be more restrict to limit context to be a required arg and at the first position like all other decorators do?

I waffled on this a bit. Is there a particular reason you think it would better to make it required? The reason I ended up leaning in this direction is that my belief is that best practice for a lakehouse computation is _not_ to access the context - it makes it more likely that the user is accessing some external resource and making their code non-deterministic. It also means the computation is easy to test without providing a bunch of scaffolding.

I kinda feel it's less robust to surface the variable by the literal arg name "context" and it could be consistent to all other entities (e.g. solid, resource, hook, etc) if the context is always the first positional arg.
my general sense was it's better to be restrict first and then lose the constraint than the opposite. I don't feel strongly about it in this case tho -- i trust your call

This revision is now accepted and ready to land.Oct 2 2020, 6:17 PM