BAI-1246 Python Logging #1284
BAI-1246 Python Logging #1284
Conversation
bw27492
requested review from
GB27247 and
a3957273
and removed request for
GB27247
There was a problem hiding this comment.
Logging should not be configured on a library. But instead should be used by developers wanting to debug code in application bound tasks. To prevent general users getting log messages we can add this line to the top level initialiser. logging.getLogger(__name__).addHandler(logging.NullHandler())
There is some good information on logging with libraries here.
Developers can then use something like this to enable logging:
import logging
logging.basicConfig()
logging.getLogger('bailo').propagate = True
lib/python/src/bailo/core/agent.py
Outdated
| @@ -25,9 +29,12 @@ def __init__( | |||
| """ | |||
| self.verify = verify | |||
|
|
|||
| logger.debug("Base Agent initiated successfully.") | |||
There was a problem hiding this comment.
Normally logging is used for unexpected behaviour and noteworthy events. I think we should use this sparingly and not for stuff such as creating objects.
There was a problem hiding this comment.
Done - object creation logs removed. Other debug messages changed to info
|
|
||
| latest_release = max(releases) | ||
| logger.info(f"Latest release ({str(latest_release.version)}) for {self.model_id} retrieved successfully.") | ||
|
|
||
| return max(releases) |
There was a problem hiding this comment.
Could we change this to latest_release?
lib/python/src/bailo/helper/model.py
Outdated
| @@ -319,6 +339,14 @@ def from_mlflow(self, tracking_uri: str, experiment_id: str): | |||
| if ml_flow: | |||
| client = mlflow.tracking.MlflowClient(tracking_uri=tracking_uri) | |||
| runs = client.search_runs(experiment_id) | |||
| if len(runs) > 0: | |||
There was a problem hiding this comment.
We can get rid of >0 It doesn't have any effect on this condition.
There was a problem hiding this comment.
Done - there was a few instances of this so I've changed them all
| logger.info( | ||
| f"Downloading {len(file_names)} of {len(orig_file_names)} files for version {str(self.version)} of {self.model_id}..." | ||
| ) |
There was a problem hiding this comment.
We can use f-strings with this although logging has the intended behaviour of formatting additional arguments into the log. We can use something like this. This applies to all the other places this is used.
logger.info(
f"Downloading %d of %d files for version %s of %s", len(file_names), len(orig_file_names), str(self.version), self.model_id
)
lib/python/src/bailo/core/agent.py
Outdated
| except KeyError: | ||
| logger.error("Access key not found in BAILO_ACCESS_KEY environment variable. Requires user input.") |
There was a problem hiding this comment.
Normally logging errors are given when something is unrecoverable within the application. I think info should be used here instead
lib/python/src/bailo/core/agent.py
Outdated
| def __request(self, method, *args, **kwargs): | ||
| kwargs["verify"] = self.verify | ||
|
|
||
| logger.debug(f"{method} to {args[0]}") |
There was a problem hiding this comment.
I don't think this is needed. Requests provide there own logging that does the same thing but with a bit more detail in terms of sessions and ports.
| name = os.path.split(path)[-1] | ||
|
|
||
| if data is None: | ||
| if is_zip := os.path.isdir(path): | ||
| logger.warning(f"Given path ({path}) is a directory. This will be converted to a zip file for upload.") | ||
| warnings.warn(f"Given path ({path}) is a directory. This will be converted to a zip file for upload.") |
There was a problem hiding this comment.
I don't think this is a warning. Probably should be information as it's expected behaviour.
lib/python/src/bailo/__init__.py
Outdated
| logging.basicConfig() | ||
| logger = logging.getLogger(__name__) | ||
| logger.addHandler(logging.NullHandler()) | ||
| logger.propagate = False |
There was a problem hiding this comment.
basicConfig is redundant as your setting a NullHandler already. Propagate is not needed either. This attribute will just propagate logs to higher loggers. Since this is top level it's redundant. logging.getLogger(__name__).addHandler(logging.NullHandler()) should be sufficient.
| warnings.warn( | ||
| f"MLFlow experiment {experiment_id} does not have any runs and publishing requires at least one valid run. Are you sure the ID is correct?" | ||
| ) |
There was a problem hiding this comment.
This should probably be info level as this is an expected behaviour.
There was a problem hiding this comment.
This is not expected behaviour - MLFlow does not return an error if you request an invalid experiment, instead it will send an empty list. Hence this warning
No description provided.