logger api #50
logger api #50
Conversation
| """Initializes the BaseLogger with a specified log directory. | ||
|
|
||
| Args: | ||
| log_dir (str): Directory to store the logs. If None, uses default log directory. |
There was a problem hiding this comment.
If you add type hints in the function signatures, we can remove the redundant types in the docstring.
| def log_metric(self, key, value, step=None): | ||
| """Logs a metric value. | ||
|
|
||
| Args: | ||
| key (str): The name of the metric. | ||
| value (float): The value of the metric. | ||
| step (int, optional): The step number at which the metric is logged. | ||
| """ | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Question: do you want every child class to have log_metric and log_params? If so, I would make BaseLogger abstract and decorate these abstractmethod instead. If, however, there are some situations where child classes shouldn't necessarily implement these functions, then it's fine to leave it as-is. I don't know what vision we have for the logging API.
| ) | ||
|
|
||
| # Save the log in the current directory | ||
| log_dir = os.path.join(os.path.abspath(os.getcwd()), "logs") |
There was a problem hiding this comment.
nit: in most of this repo, we use pathlib.Path. For consistency, it would be nice to change the os-based code to instead use Path. If you don't want to do it, it's also totally fine.
| logger = LoggerFactory.get_logger("tensorboard", log_dir) | ||
|
|
||
| # Define a callback to log progress | ||
| times = [datetime.now()] |
There was a problem hiding this comment.
FYI: at time of review, I don't see a callback here. I also haven't run the code myself since this isn't ready for review yet, so I could just be misunderstanding how the logging works in this example.
Co-authored-by: alberthli <alberthli@caltech.edu>
Implementing a base logger class that support custom logger backends.