search

awslabs / multi-model-server

Multi Model Server is a tool for serving neural net models for inference
Apache License 2.0
984 stars 230 forks source link

Default log level change from 1.1.4 -> 1.1.6 #987

Closed kastman closed 2 years ago

kastman commented 2 years ago

Hi @maaquib -

Thanks for updating to log4j2 and patching the recent vulnerability. When using the new version, however, it seems the default log level has now been set very sparsely, so that all the previous stdout logging is not shown.

Can you add an example to the logging doc to specify what the previous level was set to, and how to return it? I'm happy to supply a PR but I'm not familiar with what the previous levels were and am not currently specifying any config.properties file at all, so it will be an intro level explanation. Thanks,

VukW commented 2 years ago

Hi. I faced similar issue in a bit more complex setup:

multi-model-server==1.1.4
sagemaker-inference==1.5.6

I am building container and deploying them in AWS SageMaker (checking logs via CloudWatch). After bumping dependencies versions to latest versions on 23, Dec, 2021

multi-model-server==1.1.7
sagemaker-inference==1.5.8

I have faced similar issue: no logs were pushed to CloudWatch anymore, it looked like container stopped writing INFO logs. Firstly I also thought something was broken in mms updates.

Exploration

In my case the actual reason was not in a single package but in combinations of them. multi-model-server 1.1.4->1.1.5 moved from log4j properties config format (log4j.properties) to log4j2 xml format (log4j2.xml). So optional --log-config parameter in multi-model-server application expects now config in xml format. However sagemaker-inference==1.5.8 still uses hardcoded log4j.properties config file. So, finally, log4j was not properly configured, having no loggers / appenders set up. So, to fix that, I need either

  1. fix sagemaker-inference package to change log config format from .properties to .xml;
  2. or fix sagemaker-inference package to be able to use custom log config instead of hardcoded one (and create proper .xml config also).

Solution

The right way is to create a PR... But actually I just forked sagemaker-inference and fixed it in my own branch :quick-and-dirty: https://github.com/VukW/sagemaker-inference-toolkit/tree/1.5.8-dev1 . Maybe I create a PR later...

P.S. why I think my solution is connected to the issue: because actually multi-model-server==1.1.7 uses the same logging levels as multi-model-server==1.1.4 (comparison) . You can check differences between log4j.properties and log4j2.xml and see that logging levels are still the same for every logger. So, in your case logs issue may araise from another package / dependency also.

kastman commented 2 years ago

@VukW - Thanks for posting that; I'm also using sagemaker-inference toolkit and had just noticed the format changed from properties to xml before New Years, so our use-cases are very similar. Thanks for the thorough description!

Did you make a proper xml file, or did you simply disable it and use the verbose mulit-model-server default? I agree we should PR back up to sagemaker-inference; do you want help with that? Thanks

maaquib commented 2 years ago

@kastman Thanks for reporting this issue. @VukW Thanks for the rca. Will get this fixed.

maaquib commented 2 years ago

@VukW Would you like to send your fix as a PR to https://github.com/aws/sagemaker-inference-toolkit/