Improve template logging and cookbook

[CaselIT]

While using alembic I came across two areas that I think could be improved:

• in the template evn.py before setting the log config we should check if it's not already setup.
  I think that we can do it by checking if the root logger has an handler configured.
  This is useful if alembic is used programmatically, since it avoids resetting the program logging config.
  I guess that an option may comment may be enough for this, if the update in the template is too much
• Add a cookbook recipe on how to add a non nullable column without a server default value

[CaselIT]

Regarding the second point, there are several options:

• add the column with a server_default, then remove it
• add the column as nullable, fill it with data using an update, then turn on nullable

Surely some other option I did not think of.

Is there a preferable way or both could/should be added? The second one is more powerful since we can also use conditions/case when satting the value

[zzzeek]

people that want to add non-null columns often want to create a whole new table and then copy the old one over and rename. this is because the migrations can lock the table and bring down a live site under load for a large set of data.

generally though for small things number 2 is likely more common, as removing server defaults can be a little weird for some backends.

[zzzeek]

i think the important thing to note is that adding non-nullable cols is difficult to make work for an application that is running as the migrations go on.

[zzzeek]

for the logging thing I dont know how logging would be set up already unless they are using the command environment progammatically, is that your use case? how often does this get in your way that you notice logging is being configured twice when first setting up a project and you have to modify env.py (and if you are using programmatic config how are you not having to modify env.py for other reasons anyway)?

[CaselIT]

Regarding non nullable then I guess we should suggest number 2, and add a warning regarding running applications with the copy etc.

[CaselIT]

unless they are using the command environment programmatically, is that your use case?

yes that is my use case. Like when creating a new db from scratch and stamping it with the current version.
And yes, I had changed the env to make it use the correct engine configuration from the application

[CaselIT]

Maybe a simple comment would suffice, so that when an user first inits a project and creates the env and looks at it to adapt it to his application, can think if the logging check should be added in his use case.

[CaselIT]

people that want to add non-null columns often want to create a whole new table and then copy the old one over and rename. this is because the migrations can lock the table and bring down a live site under load for a large set of data.

I don't think we want to suggest this one, just warn of the possible lock of the table during the update, since particular care must be placed to ensure that no data is lost in a live application. Like all the data that is committed to the original table from the start of the copy to the rename would be lost if particular care is not used in this case

[BrianMSheldon]

The logging just bit me today as I was attempting to apply the cookbook recipe for creating a new database from scratch. My initial assumption was that the database creation hung since that was the last logging produced. It didn't take long to realize it had disabled the logger on execution, but a note to that affect in the cookbook would have saved some frustration.

[charlax]

This seems to be a recurring question: see #795 and #714 for instance.

A comment and a default check for the root logger's handlers' presence would be great. For instance:

# Interpret the config file for Python logging if it is not already configured
if not logging.getLogger().handlers:
    assert config.config_file_name
    fileConfig(config.config_file_name)

[zzzeek]

im -1 on that because there are also formatters in fileConfig as well as handler configurations for other loggers. IMO people need to change their env.py if they dont want the logging config to be used.

[epizut]

@zzzeek are you sure about that? I am facing the same issue here: mlflow/mlflow#13464
Note that the official logging documentation states that:

It is strongly advised that you do not add any handlers other than NullHandler to your library’s loggers. This is because the configuration of handlers is the prerogative of the application developer who uses your library. The application developer knows their target audience and what handlers are most appropriate for their application: if you add handlers ‘under the hood’, you might well interfere with their ability to carry out unit tests and deliver logs which suit their requirements.

Knowing that, I think wrapping the fileConfig() call into if not logging.getLogger().handlers: looks like a proper solution to me.