Page MenuHomePhabricator

move compute log manager from graphql context to dagster instance
ClosedPublic

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

Details

Summary

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

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
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

python_modules/dagster/dagster/core/instance/__init__.py
145

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?

347–348

undebug

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.
python_modules/dagster/dagster/core/execution/logs.py
42–44

😐

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
python_modules/dagster/dagster/core/execution/logs.py
42–44

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.