Page MenuHomePhabricator

move compute log manager from graphql context to dagster instance

Authored by prha on Sep 13 2019, 11:12 PM.



Instead of putting the ComputeLogManager in the graphql context and
threading the DagsterInstance through, this diff places the ComputeLogManager
on the DagsterInstance itself and changes the compute log interactions to
coordinate all log capture and log fetching through the manager

Test Plan

ran tests, saw interactive run with streaming logs

Diff Detail

R1 dagster
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

prha created this revision.Sep 13 2019, 11:12 PM
prha updated this revision to Diff 4756.Sep 13 2019, 11:42 PM

update missing test argument

prha added a reviewer: Restricted Project.Sep 13 2019, 11:58 PM
alangenfeld requested changes to this revision.Sep 17 2019, 4:23 PM
alangenfeld added a subscriber: alangenfeld.

to your queue


I think we should have a no-op log manager or support None here - did you test how much this slowed down test runs to be writing all this stuff to disk?



This revision now requires changes to proceed.Sep 17 2019, 4:23 PM
prha updated this revision to Diff 4834.Sep 18 2019, 7:07 PM

disable compute log manager in ephemeral instances (pytests)

alangenfeld accepted this revision.Sep 18 2019, 7:51 PM
alangenfeld added inline comments.


This revision is now accepted and ready to land.Sep 18 2019, 7:51 PM
prha added inline comments.Sep 18 2019, 8:18 PM

yeah, to be replaced by NoOpComputeLogManager when we move to the abstract manager

This revision was landed with ongoing or failed builds.Sep 18 2019, 8:18 PM
This revision was automatically updated to reflect the committed changes.