Page MenuHomeElementl

[crag] ugprade toys callsites
AbandonedPublicDraft

Authored by alangenfeld on May 24 2021, 9:02 PM.

Details

Summary

cragify toys

Test Plan

bk

Diff Detail

Repository
R1 dagster
Branch
crag
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

In my opinion that pattern should be that the with_resources call happens right in the argument where the repository rather than redefining the global symbol. You want a separate global symbol for testing graphs directly. Downside of that approach is lack of ability to load dagit directly from file.

Harbormaster returned this revision to the author for changes because remote builds failed.May 24 2021, 9:22 PM
Harbormaster failed remote builds in B31088: Diff 38239!

In my opinion that pattern should be that the with_resources call happens right in the argument where the repository rather than redefining the global symbol. You want a separate global symbol for testing graphs directly. Downside of that approach is lack of ability to load dagit directly from file.

Redefining the global symbol seems pretty yucky to me. If someone skims to code, but doesn't get to the end, they'll expect longitudinal_pipeline to be a graph, when it's actually an executable/job/target.

Invoking with_resources where the repository is defined sounds fine here, but I'm ambivalent about that as a general recommendation. It's often nice to encapsulate all the stuff that's specific to a particular job in a single module. If someone has a set of prod resources that they want to use across a bunch of graphs, then I think it's sensible to apply those resources in the module that defines the repository. But some people might not see/care about the distinction between a logical graph and a job, so constructing the job across two files might feel weird for them.

python_modules/dagster-test/dagster_test/toys/composition.py
13

Could omit context argument.

44

Did this not raise a pylint issue before?

Harbormaster returned this revision to the author for changes because remote builds failed.May 27 2021, 10:21 PM
Harbormaster failed remote builds in B31375: Diff 38614!