Added logging root directory #190
Conversation
src/kagglehub/logger.py
Outdated
| @@ -35,7 +38,8 @@ def _configure_logger(log_dir: Optional[Path] = None) -> None: | |||
| handler = library_logger.handlers.pop() | |||
| while handler.filters: | |||
| handler.filters.pop() | |||
| log_dir = Path.home() / ".kaggle" / "logs" if log_dir is None else log_dir | |||
| log_root_dir = Path(os.environ.get(KAGGLE_LOGGING_ROOT_DIR_ENV_VAR_NAME, Path.home())) | |||
There was a problem hiding this comment.
Thank you for working on the fix. Correct me if I am wrong:
I think the user mentioned (#137) that Path.home() might throw exceptions during "import", and using an env variable might be more complicated.
In that case, do you think it's simpler to use some standard ways such as /var/log or /tmp?
There was a problem hiding this comment.
Unix style paths aren't available on windows which are a subset of our users.
There was a problem hiding this comment.
If we start customizing the logic based on os, we would end up building something like Path.home()
There was a problem hiding this comment.
I see. Probably makes sense to try and except and disable the logging if Path.home or /var/log doesn't work at all.
And +1 to make it opt-in as @rosbo mentioned below.
|
Should logging to a file be disabled by default? And we could add an environment variable to opt-in? You really only need to look at these logs if you are facing an issue and need to debug it. |
Made file logging optional. |
Provides workaround for issue: #137
BUG=b/382252531