vanna icon indicating copy to clipboard operation
vanna copied to clipboard

Add standardized and overridable logging

Open andreped opened this issue 1 year ago β€’ 3 comments

This PR adds standardized, overridable logging as referred to in issue #176.

The solution is using native python logging. I only added the simple logging solution and replaced all print statements with the appropriate logging type.

I tested the solution using the Getting_started.ipynb Jupyter Notebook. Logging seems to work as intended.

CC: @justbee007, @zainhoda

andreped avatar Feb 05 '24 19:02 andreped

I realized that setting the log level does not require its own method. You can simply do:

import vanna as vn

vn.logger.get_logger().set_level(level="INFO")

Perhaps we should update the documentations regarding this? Or do we want anything fancier than this.

Perhaps we want to support an API like this instead:

import vanna as vn

vn.get_logger().setLevel(level="INFO")

This is similar to what TensorFlow does, or at least used to do. Looks nicer, IMO.

Or should we go even simpler:

import vanna as vn

vn.setLogLevel(level="INFO")

Maybe call the method something else, like setVerbosity?

Suggestions, @zainhoda?

andreped avatar Feb 05 '24 20:02 andreped

I added that setVerbosity convenience method as discussed above. So now logging verbosity can be easily set by:

import vanna as vn

vn.setVerbository(level="DEBUG")

By default the logging level is "INFO".

andreped avatar Feb 05 '24 21:02 andreped

@NickCrews @andreped my apologies for the delay here. I haven’t yet had a chance to keep up with this PR and the thread. I’ll review this weekend and come back with thoughts. Thank you so much!

zainhoda avatar Feb 16 '24 16:02 zainhoda

@NickCrews @andreped thank you so much for your contributions here. If you don't mind, I would like to put a temporary "hold" on this feature while we define it a little more. I'm going to take the suggestions you've come up with and use that as a basis for how we implement this. I'll submit an independent PR for this but I'll make sure you're in the loop.

To give you a sense of what I'm thinking:

  • We're moving to a model where vn is always an instantiated object that inherits (directly or indirectly) from the VannaBase class
  • Behavior customizations to Vanna are done by overriding certain methods or potentially adding new methods to the class
  • I think logging should function in a similar manner where the default option is to print the output and the user can override the method to do "something else" with the logs
  • A couple of the options for the logs should be sending them to a database as well as sending them to an optional Vanna endpoint. I think this is particularly helpful for reviewing the history of fully hydrated prompts. To do this well, I'll be looking to add some additional parameters to the logging function so that these fields can be routed to a database. A few fields that come to mind are timestamp, question_id, method in addition to the log level and the actual contents of the log.

I might also need to add some functions to retrieve the logs although I'll give that some more thought. Basically I want to be able to optionally support viewing the logs associated with a particular question within the UI

zainhoda avatar Feb 21 '24 15:02 zainhoda

thank you so much for your contributions here. If you don't mind, I would like to put a temporary "hold" on this feature while we define it a little more. I'm going to take the suggestions you've come up with and use that as a basis for how we implement this. I'll submit an independent PR for this but I'll make sure you're in the loop.

@zainhoda Sounds good. Your argument is sound. Feel free to close this PR and tag it when you have opened a separate PR :]

andreped avatar Feb 21 '24 16:02 andreped

@zainhoda I believe there has been done some work on this already, which have already been merged. The original issue will remain open till the issue is entirely closed, but I am closing this PR, as this proposal will not be used.

andreped avatar Mar 18 '24 11:03 andreped