Page MenuHomeElementl

initialize method for logger that enables default config
ClosedPublic

Authored by cdecarolis on Thu, May 27, 4:31 PM.

Details

Summary

Add an initialize method to logger definition that allows us to map and validate default config before passing to logger_fn.

Test Plan

expanded unit tests.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

add UnboundInitLoggerContext, which gets bound into InitLoggerContext during initialization

i guess initialize is better than logger_fn but [1]

python_modules/dagster/dagster/core/definitions/logger.py
96–111

hmmmmmmmmmmm

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_build_init_logger_context.py
4–9

[1] why cant i just invoke the function str_logger that i wrote?

str_logger(build_init_logger_context())

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_build_init_logger_context.py
4–9

I think the argument against is that str_logger does not behave like a function in other contexts and is not named like a function. A solid feels like a function. You'd probably give it a name that's a verb, not a noun, e.g. compute_x. There are places where you treat it like a function like by invoking it during composition. On the other hand, someone seeing ModeDefinition(logger_defs=[my_logger]) would not expect that my_logger is a function.

python_modules/dagster/dagster/core/definitions/logger.py
43–73

*

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_build_init_logger_context.py
4–9

I see the case, but i feel like if you are testing it directly like this theres a good chance you wrote it, and if you wrote it you know its a decorated function and may be looking to just invoke it.

In fact you can invoke it * it just behaves in an unintuitive way

should we change the __call__ impl to go down this path in addition to having a method like initialize ?

python_modules/dagster/dagster/core/definitions/logger.py
96–111

¯\_(ツ)_/¯

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_build_init_logger_context.py
4–9

The fact that this logger defs do literally need to be callable is a point in favor of just making them callable, in my book. Why have both methods if call is gonna redirect to initialize? Calling will always be more convenient

Respin with call

Updated to use invocation instead of initialize

This revision is now accepted and ready to land.Thu, Jun 3, 12:20 AM