Uvicorn logging settings

[pablobgar]

Closes #530

add support to uvicorn logging setting:

• add a parameter to define a logging configuration file into application settings
• edit uvicorn Config initialization to add this parameter

[adriangonz]

Hey @pablobgar ,

Thanks for opening this PR! It's really great to see these community-driven efforts.

Just so that I understand a bit more about Uvicorn's logging config file, wouldn't those settings apply to every logging handler configured in the process? Or does Uvicorn filter out the handlers and formatters to only apply the ones that it will use? Looking at this issue, I can see that the logging config file seems quite generic, so I'd assume that it gets applied globally?

[pablobgar]

Hi @adriangonz,

Thanks for your answer.

You are right, this logging config file is applied globally. Uvicorn applies it by calling python's logging.config.fileConfig which applies it globally. Anyway, in the file you can specify that the configuration affects only certain loggers, here you can see an example of it.

Maybe this is not a good solution, I know it can be quite confusing. We can try to find a way to make a more generic approach to allow users to configure custom handlers on all application loggers.

Any ideas?

[adriangonz]

Hey @pablobgar ,

Thanks for that clarification!

Based on that, would it make sense to change the setting name to logging_settings so that it's clear that users can leverage it for the global logging config? We could also apply that ourselves explicitly (or just leverage the fact that uvicorn already applies that globally).

[pablobgar]

Hello @adriangonz,

I agree we should change the name to logging_settings.

I think it would be a good idea to explicitly apply the configuration ourselves to avoid having that dependency with Uvicorn. In any case, we have to apply it in Uvicorn as well.

I have made a new commit to rename the variable and to apply the configuration file on logging.py. At now, it supports python's logging.config format, but we can also add support to JSON and YAML as they do in this uvicorn's function if you think it is useful.

[adriangonz]

That looks fantastic @pablobgar! I think it makes a lot of sense to apply them globally. Thanks a lot for pushing this one upstream! 🚀

Once the tests are green, we should be good to go.

[adriangonz]

Alright, it seems like there's a small linter error (an unused import). Once that's resolved, this should good to go ahead @pablobgar 👍

mlserver/logging.py:19: error: Module has no attribute "config"

(no need to worry about the docs error BTW - that was due to an outdated dep and should be fixed already in master)

[adriangonz]

Hey @pablobgar ,

It looks like the linter has failed again. If you want to run this locally, it should be possible to use the make lint target (which would run both the linter and mypy).

[pablobgar]

Alright, it seems to have worked now!

I had some troubles with Optional[str] type and the linter... 😅

What are the next steps @adriangonz?

[adriangonz]

Nice one @pablobgar! Thanks a lot for being patient with the linter!

Alright, this one should be good to go now 🚀

[pablobgar]

Hi @adriangonz !!!

Good to see this feature merged!!! When is a release coming out with these changes?

[adriangonz]

We're currently working towards the 1.1.0 milestone. You can see on the link below the current progress and what's still left to get there:

https://github.com/SeldonIO/MLServer/projects/2?query=is%3Aopen+sort%3Aupdated-desc