RFC: Change logger behavior so that setting a default logger in a Definitions object changes the logger by default, without needing to set run config to enable it #24338
Conversation
…ons object actually sticks, without needing to set run config Summary: Right now if you customize a logger on a job, you also need to set additional configuration on the run when launching the job in order for it to actually use that logger and not fall back to the detault system logger. This is quite different than what happens when you set an executor on a job, and is generally confusing. This PR instead makes setting logger config more like executor config - if you set it, it is assumed that you want to use that logger, and applies that configuraiton as the default even if no configuration is set. The main blocker here is probably that this could be considered a breaking change - before, when you specified a logger on a job you were giving your self the option of opting into it, and now you are overriding whatever loggers you set as the default loggers.
gibsondan
changed the title
|
One option to make this less of a breaking change would be to enable this new behavior specifically in the case where a logger is defined at the Definitions level and is injected into the job. We explicitly claim that this is a way to set a "default logger" here: https://docs.dagster.io/concepts/logging/loggers#specifying-default-code-location-loggers- (and claim in the docs that that is "experimental", although i don't see the parameter annotated as experimental) - this would be just making that actually accurate. It is a bit odd to treat the Definitions case differently from the case where you set a logger at the job level though. |
|
This seems eminently sensible to me from a starting-from-scratch perspective, but complicated from a backcompat perspective. I'm having a little trouble following what's going on in this PR - it seems like most of the changes are around threading through the instance? Is that a core part of the change here? A small test might help make the contours of the user-facing behavior change more clear? |
|
I can split out the threading the instance part. The main reason that was needed is that the "default system" colored console logger defaults to DEBUG, but if you specify it in code, the default level is INFO. And we log all of our dagster system events to the command line at DEBUG level. So swapping it in to use the default 'code' colored console logger instead causes those log lines to disappear unless we change the default log level behavior to match between the "system" logger and the "code" logger (which requires looking at the instance). |
Summary:
Right now if you customize a logger on a job, you also need to set additional configuration on the run when launching the job in order for it to actually use that logger and not fall back to the detault system logger. This is quite different than what happens when you set an executor on a job, and is generally confusing.
This PR instead makes setting logger config more like executor config - if you set it, it is assumed that you want to use that logger, and applies that configuraiton as the default even if no configuration is set. Much like with executors, this could cause the job config to be invalid if the logger has non-default config options and they are not set (none of our default loggers have this property, but e.g. cloudwatch_logger in dagster-aws does, and I could imagine user-defined loggers having them as well).
The main blocker here is the potential for this to be breaking change - before, when you specified a logger on a job you were giving your self the option of opting into it, and now you are overriding whatever loggers you set as the default loggers. Perhaps there is some way to make this behavior change opt-in to alleviate the pain before changing the default behavior?
Summary & Motivation
How I Tested These Changes
Changelog
Insert changelog entry or "NOCHANGELOG" here.
NEW(added new feature or capability)BUGFIX(fixed a bug)DOCS(added or updated documentation)