Page MenuHomePhabricator

Refactor compute log watching contextmanager to move to the compute log manager
ClosedPublic

Authored by prha on Fri, Mar 20, 9:24 PM.

Details

Summary

We want to begin capturing pipeline-level compute logs, not just the individual step
compute. This diff refactors a lot of the context management to the compute log manager, so
that we can become aware of potentially nested calls to the compute log manager within a given
process.

A few things are included in this diff:

  1. Moves the mirror_step_io context manager to be the watch instance method on the ComputeLogManager.
  2. Restructures the compute_logs.py function apis to be based on files and streams instead of ComputeIOType, to clarify the test surface area
  3. Deprecates the dagster utils cli commands, in favor of a dedicated, thinner utility script used to poll compute logs on Windows (where tail is not available)

This is the base diff for further work, including nesting watch calls, with the outer pipeline-level watch happening in the core.execution.api api methods. This would then capture all of the pipeline logs (including non-step-specific logging), in a single pipeline log file.

Test Plan

added some bk test cases

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.Fri, Mar 20, 9:24 PM
prha updated this revision to Diff 10782.Fri, Mar 20, 11:08 PM

update tests

prha updated this revision to Diff 10783.Sat, Mar 21, 1:13 AM
  • fix py27 compatible test
prha updated this revision to Diff 10785.Sat, Mar 21, 2:35 AM
  • fix py27 compatible test
prha edited the summary of this revision. (Show Details)Sat, Mar 21, 10:25 PM
prha added reviewers: alangenfeld, schrockn, max.
alangenfeld accepted this revision.Mon, Mar 23, 6:48 PM

this is great - just a little clean up before land

python_modules/dagster/dagster/core/engine/engine_inprocess.py
103–105 ↗(On Diff #10785)

nice i like this a lot

python_modules/dagster/dagster/core/execution/compute_logs.py
119

whats up with this?

python_modules/dagster/dagster/core/storage/compute_log_manager.py
67

nit: rm "to the filesystem" since other implementors my persist elsewhere

python_modules/dagster/dagster/core/storage/local_compute_log_manager.py
40–56

chefkiss

python_modules/dagster/dagster_tests/core_tests/test_stdout.py
88

skeptical

This revision is now accepted and ready to land.Mon, Mar 23, 6:48 PM
prha updated this revision to Diff 10910.Mon, Mar 23, 10:33 PM
prha edited the summary of this revision. (Show Details)

fix remote s3 implementation, alangenfeld comments, resolve conflicts