-
Notifications
You must be signed in to change notification settings - Fork 274
refactor(pathfinder): replace info_summary_append fixture with Python logging #1593
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
Changes from all commits
dad96a5
e83b6f5
ee20cd6
37044ad
80b7c48
93373c4
db45d36
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,33 +2,47 @@ | |
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
|
|
||
| import logging | ||
| import os | ||
|
|
||
| import pytest | ||
|
|
||
| _LOGGER_NAME = "cuda_pathfinder.test_info" | ||
|
|
||
|
|
||
| def _log_filename(): | ||
| strictness = os.environ.get("CUDA_PATHFINDER_TEST_LOAD_NVIDIA_DYNAMIC_LIB_STRICTNESS", "see_what_works") | ||
| return f"pathfinder-test-info-summary-{strictness}.txt" | ||
|
|
||
|
|
||
| def pytest_configure(config): | ||
| config.custom_info = [] | ||
| log_path = config.rootpath / _log_filename() | ||
| log_path.unlink(missing_ok=True) | ||
|
|
||
|
|
||
| def pytest_terminal_summary(terminalreporter, exitstatus, config): | ||
| if not config.getoption("verbose"): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, what about deleting only the However, we should unconditionally remove the file when the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's getting overwritten 99 times?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, that's basically spamming the filesystem. |
||
| return | ||
| if hasattr(config.option, "iterations"): # pytest-freethreaded runs all tests at least twice | ||
| return | ||
| if getattr(config.option, "count", 1) > 1: # pytest-repeat | ||
| return | ||
| @pytest.fixture(scope="session") | ||
| def _info_summary_handler(request): | ||
| log_path = request.config.rootpath / _log_filename() | ||
| handler = logging.FileHandler(log_path, mode="w") | ||
| handler.setFormatter(logging.Formatter("%(test_node)s: %(message)s")) | ||
|
|
||
| if config.custom_info: | ||
| terminalreporter.write_sep("=", "INFO summary") | ||
| for msg in config.custom_info: | ||
| terminalreporter.line(f"INFO {msg}") | ||
| logger = logging.getLogger(_LOGGER_NAME) | ||
| logger.addHandler(handler) | ||
| logger.setLevel(logging.INFO) | ||
| logger.propagate = False | ||
|
|
||
| yield handler | ||
|
|
||
| @pytest.fixture | ||
| def info_summary_append(request): | ||
| def _append(message): | ||
| request.config.custom_info.append(f"{request.node.name}: {message}") | ||
| logger.removeHandler(handler) | ||
| handler.close() | ||
|
|
||
| return _append | ||
|
|
||
| @pytest.fixture | ||
| def info_log(request, _info_summary_handler): | ||
| return logging.LoggerAdapter( | ||
| logging.getLogger(_LOGGER_NAME), | ||
| extra={"test_node": request.node.name}, | ||
| ) | ||
|
|
||
|
|
||
| def skip_if_missing_libnvcudla_so(libname: str, *, timeout: float) -> None: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I posted #1621.
Introducing these artifacts makes something simple weirdly complex. The INFO lines are a tiny fraction of the total log (see PR #1621 description for numbers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still too noisy, and the mechanism that we are using to track this information lacks any sort of real structure. It's very ad-hoc and can't really ever be integrated into or compose well with another system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point to a concrete, measurable problem (similar to the numbers I showed under #1621)?
I'm using those
INFOlines regularly when working on pathfinder, or if random issues pop up. Hiding that information in disconnected artifacts would force me to puzzle together information from pieces that was perviously available at a glance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a problem in CI anymore it seems. It's still a big code smell though. Is that measurable? Not exactly. Does it have to be to be a valid concern? Definitely not. Closing the PR.