Page MenuHomeElementl

convert local compute log manager
Needs ReviewPublic

Authored by prha on Jul 12 2021, 9:51 PM.

Details

Summary

Depends on D8385. Changes the local compute log manager to implement
the CapturedLogManager interface, using multiple inheritance.

Test Plan

bk

Diff Detail

Repository
R1 dagster
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

prha requested review of this revision.Jul 12 2021, 10:35 PM
python_modules/dagster/dagster/core/storage/local_compute_log_manager.py
32

does this need to impl both? since this is in the core package it should trigger only the new behavior right?

147–152

whats up here?

python_modules/dagster/dagster_tests/core_tests/storage_tests/utils/captured_log_manager.py
40–41

early return in test seems spicy - comment at least but maybe remove? seems like a gnarly thing if it triggers unintentionally and all the tests pass

72–73

^

python_modules/dagster/dagster/core/storage/local_compute_log_manager.py
140

whats the thinking of having this default to true & having it configurable versus upgrading the remote compute log managers to just return False for capture by step

python_modules/dagster/dagster/core/storage/local_compute_log_manager.py
32

It doesn't need to implement both, but I'm wary of folks who've built their own implementation on top of the ComputeLogManager methods. Might make sense to strip it out later, after some time with a deprecation warning?

140

Just not sure about the behavior change of splitting it out by process or not. I could go either way, to be honest, though.

147–152

For captures outside of a pipeline run context, we might not have a run_id supplied. os.path.join will error out if run_id is None because it's expecting a string.

python_modules/dagster/dagster_tests/core_tests/storage_tests/utils/captured_log_manager.py
40–41

Yeah, I'm just going to get rid of this

python_modules/dagster/dagster/core/storage/local_compute_log_manager.py
140

hmmmm something to talk through

147–152

ah - mypy I think would make it clear

python_modules/dagster/dagster/core/storage/local_compute_log_manager.py
140

yeah, maybe seems more like a no-brainer if I had gotten this in for 0.12.0.

more mypy, renaming for clarity