Skip to content

Configure log path via env var, and catch filesystem errors #975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jul 30, 2020

Conversation

dsherry
Copy link
Contributor

@dsherry dsherry commented Jul 24, 2020

Fix #956

We'll now support the ability to a) configure and disable the log file and b) ignore file logging if there are filesystem permissions errors during setup.

Behavior

  • Default: As before, if the env var EVALML_LOG_FILE is not set, the default behavior will be to write logs to evalml_debug.log in the current directory.
  • Configurable log location: If the env var EVALML_LOG_FILE is set to a valid file path before importing evalml, evalml will write debug-level logs to that location.
    • This will occur regardless of whether the file specified already exists, as long as the parent directory exists and is writeable.
  • Error handling: If at any point the chosen path is a) a directory or b) the program doesn't have write access in the parent directory, a warning and stacktrace will be logged, and logger setup will continue without a debug log file
  • Disabling: if the env var EVALML_LOG_FILE is set to an empty value, logging setup will not attempt to create a log file.

Examples
Default behavior: log to evalml_debug.log in current repo

python demo.py

Log to a different location (must be filepath, not dir):

EVALML_LOG_PATH="/path/to/logfile.log" python demo.py

To suppress log output entirely:

EVALML_LOG_PATH="" python demo.py

@dsherry dsherry force-pushed the ds_956_logger_permissions_fix branch from 4b1121f to 25d37cf Compare July 24, 2020 14:34
@codecov
Copy link

codecov bot commented Jul 24, 2020

Codecov Report

Merging #975 into main will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##             main     #975    +/-   ##
========================================
  Coverage   99.85%   99.86%            
========================================
  Files         179      179            
  Lines        9283     9424   +141     
========================================
+ Hits         9270     9411   +141     
  Misses         13       13            
Impacted Files Coverage Δ
evalml/tests/utils_tests/test_logger.py 100.00% <100.00%> (ø)
evalml/utils/logger.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1f1adb...eada442. Read the comment docs.

@dsherry dsherry force-pushed the ds_956_logger_permissions_fix branch from 25d37cf to c04c581 Compare July 28, 2020 22:38
@dsherry dsherry force-pushed the ds_956_logger_permissions_fix branch from c04c581 to 5d0d4a8 Compare July 30, 2020 14:13
@dsherry dsherry marked this pull request as ready for review July 30, 2020 14:31
Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsherry I don't think I'm getting the intended behavior when I set EVALML_LOG_FILE to a filepath that does not exist or a directory. Let me know if I am misunderstanding!

logger.setLevel(logging.DEBUG)

evalml_log_path_str = os.environ.get('EVALML_LOG_FILE', 'evalml_debug.log')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be helpful to print the log file so the user knows where to check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, that would be helpful. However I don't think we should do that in this PR.

This code gets called once per static invocation of the logger, i.e. once per evalml source file. If we add a print here, it'll occur multiple times. I'd prefer that sort of thing to happen only once. If we want to do that, we'd need a global config check to happen before this code is ever called. We don't have a "config" object in evalml yet. I believe featuretools does.

You are reminding me: we have to add something to the user guide or API docs about logging. I will include that in this PR. That should help a lot.

Copy link
Contributor

@freddyaboulton freddyaboulton Jul 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that's a really good point. For the time being, we display the logfile path whenever we encounter an error during scoring in AutoML search so that should help the user find the logfile when they need it.

Copy link
Collaborator

@jeremyliweishih jeremyliweishih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other than the fixes that @freddyaboulton proposed!

@jeremyliweishih
Copy link
Collaborator

@dsherry might need to kick off the build_docs test again. Sometimes it does this.

@dsherry
Copy link
Contributor Author

dsherry commented Jul 30, 2020

Yeah thanks @jeremyliweishih , more circleci network hiccups. They seem to have increased a bunch as of a few weeks ago.

Copy link
Contributor

@angela97lin angela97lin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one comment about potentially using the tmp_dir fixture if applicable but otherwise LGTM :)

def test_get_logger_path_valid(mock_RotatingFileHandler, monkeypatch, logger_env_cleanup):
assert os.environ.get('EVALML_LOG_FILE') is None

with tempfile.TemporaryDirectory() as temp_dir:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure this is applicable here but there's a tmp_dir fixture you might be able to use: https://docs.pytest.org/en/stable/tmpdir.html :) We've used it in other tests for creating a temporary directory!

if len(evalml_log_path_str) == 0:
return logger
if evalml_log_path.is_dir() or not os.access(evalml_log_path.parent, os.W_OK):
print(f'Warning: cannot write debug logs to path "{evalml_log_path}". ' + warning_msg)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit ugly because it prints out once for every file in our codebase which imports and uses the logger. But its a fine starting place. Better to surface the warning here than to be silent. Thanks @freddyaboulton !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @dsherry !

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

"\n",
"EvalML uses [the standard python logging package](https://docs.python.org/3/library/logging.html). By default, EvalML will log `INFO`-level logs and higher (warnings, errors and critical) to stdout, and will log everything to `evalml_debug.log` in the current working directory.\n",
"\n",
"If you want to change the location of the logfile, before import, set the `EVALML_LOG_FILE` environment variable to specify a filename within an existing directory in which you have write permission. If you want to disable logging to the logfile, set `EVALML_LOG_FILE` to be empty. If the environment variable is set to an invalid location, EvalML will print a warning message to stdout and will not create a log file."
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a page to the guide

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a little more clear to specify that it needs to be set to an empty string

@dsherry dsherry merged commit 7b6a469 into main Jul 30, 2020
@dsherry dsherry deleted the ds_956_logger_permissions_fix branch July 30, 2020 20:02
@angela97lin angela97lin mentioned this pull request Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants