refactor(pathfinder): replace info_summary_append fixture with Python logging#1593
refactor(pathfinder): replace info_summary_append fixture with Python logging#1593cpcloud wants to merge 6 commits intoNVIDIA:mainfrom
Conversation
… logging
Replace the custom `info_summary_append` pytest fixture and terminal summary
with standard Python `logging`. Test diagnostic info now writes to a per-
strictness log file (`test-info-summary-{strictness}.log`) via a
`logging.FileHandler`, uploaded as a GHA run artifact.
- Drop `custom_info` list, `pytest_terminal_summary`, and the
`info_summary_append` fixture from conftest
- Add session-scoped `_info_summary_handler` yield fixture (FileHandler
lifecycle) and function-scoped `info_log` fixture (LoggerAdapter with
test node name)
- Update test call sites to use `info_log.info(...)` instead of
`info_summary_append(...)`
- Remove `tee` + `grep` verification from `ci/tools/run-tests`
- Add `upload-artifact` step to both test-wheel workflows with
`if-no-files-found: error`
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
/ok to test |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
/ok to test |
|
rwgk
left a comment
There was a problem hiding this comment.
I'm OK with it, although I want to note that breaking currently certainly coherent output into pieces has pitfalls. Definitely, we need to clean up at the beginning. We should also add the new log file to .gitignore.
cuda_pathfinder/tests/conftest.py
Outdated
|
|
||
| @pytest.fixture(scope="session") | ||
| def _info_summary_handler(request): | ||
| strictness = os.environ.get("CUDA_PATHFINDER_TEST_LOAD_NVIDIA_DYNAMIC_LIB_STRICTNESS", "default") |
There was a problem hiding this comment.
I think it's better and simpler to hardwire the filename. The strictness level is only for controlling hard asserts. Also, maybe make it easy to know what the file is about even when it's pulled into another context, e.g. by adding in pathfinder:
log_path = request.config.rootpath / f"pathfinder-test-info-summary-log.txt"
Naming it .txt makes for the smoothest experience viewing, uploading, etc. Other extensions tend to confuse one tool or another.
There was a problem hiding this comment.
Is there ever a case where the pathfinder tests are run twice CI? Once with see_what_works and once with the other setting?
There was a problem hiding this comment.
CI runs pathfinder tests twice per job -- once with see_what_works (test-wheel-linux.yml:250, test-wheel-windows.yml:223) and once with all_must_work (test-wheel-linux.yml:295, test-wheel-windows.yml:272). The strictness level is embedded in the filename so the second run does not silently overwrite the first run's log. pytest_configure only deletes the file matching the current strictness level, so both logs coexist for artifact upload.
cuda_pathfinder/tests/conftest.py
Outdated
| @pytest.fixture(scope="session") | ||
| def _info_summary_handler(request): | ||
| strictness = os.environ.get("CUDA_PATHFINDER_TEST_LOAD_NVIDIA_DYNAMIC_LIB_STRICTNESS", "default") | ||
| log_path = request.config.rootpath / f"test-info-summary-{strictness}.log" |
There was a problem hiding this comment.
We want to be sure we're not accidentally using a file from a previous run (e.g. if there is a segfault but some follow-on logic still looks for that file). Cursor generated the code below. If we hard-wire the filename as suggested, we don't even need the glob.
diff --git a/cuda_pathfinder/tests/conftest.py b/cuda_pathfinder/tests/conftest.py
index 2a82cca0c..a10278f2d 100644
--- a/cuda_pathfinder/tests/conftest.py
+++ b/cuda_pathfinder/tests/conftest.py
@@ -7,9 +7,15 @@ import os
import pytest
+_INFO_LOG_GLOB = "test-info-summary-*.log"
_LOGGER_NAME = "cuda_pathfinder.test_info"
+def pytest_configure(config):
+ for log_path in config.rootpath.glob(_INFO_LOG_GLOB):
+ log_path.unlink(missing_ok=True)
+
+
@pytest.fixture(scope="session")
def _info_summary_handler(request):
strictness = os.environ.get("CUDA_PATHFINDER_TEST_LOAD_NVIDIA_DYNAMIC_LIB_STRICTNESS", "default")There was a problem hiding this comment.
See #1593 (comment), it's also related to your comment here.
| handler.setFormatter(logging.Formatter("%(test_node)s: %(message)s")) | ||
|
|
||
| def pytest_terminal_summary(terminalreporter, exitstatus, config): # noqa: ARG001 | ||
| if not config.getoption("verbose"): |
There was a problem hiding this comment.
Hm, what about deleting only the verbose condition, but keeping the other two? Otherwise the file will be overwritten 99 times or so?
However, we should unconditionally remove the file when the _info_summary_handler fixture runs. (I've been really confused a few times in the past by stale output, correlating it wrongly to a more recent action.)
There was a problem hiding this comment.
What's getting overwritten 99 times?
There was a problem hiding this comment.
The _info_summary_handler fixture is scope="session", so it's created exactly once per pytest invocation regardless of how many times pytest-repeat re-runs the tests. The FileHandler opens the file once with mode="w" at session start and appends to it for the duration -- repeated tests just add more lines to the same open handle.
There was a problem hiding this comment.
Hm, that's basically spamming the filesystem.
- Hardwire log filename with `pathfinder` prefix and `.txt` extension - Retain strictness suffix so both CI runs produce separate logs - Add `pytest_configure` to clean up stale log for the current strictness level (not all levels, preserving the other run's log) - Add line count echo to `run-tests` for quick CI log grep - Add `.gitignore` entry for the new `.txt` log files - Update artifact upload paths in both workflow files - Align default strictness fallback with `test_load_nvidia_dynamic_lib` Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
/ok to test |
|
/ok to test ab07049 |
| CUDA_PATHFINDER_TEST_FIND_NVIDIA_HEADERS_STRICTNESS: all_must_work | ||
| run: run-tests pathfinder | ||
|
|
||
| - name: Upload cuda.pathfinder test info summary |
| handler.setFormatter(logging.Formatter("%(test_node)s: %(message)s")) | ||
|
|
||
| def pytest_terminal_summary(terminalreporter, exitstatus, config): # noqa: ARG001 | ||
| if not config.getoption("verbose"): |
There was a problem hiding this comment.
Hm, that's basically spamming the filesystem.
What
Replaces the custom
info_summary_appendpytest fixture with standard Pythonlogging, writing test diagnostic info to a file uploaded as a GHA artifact instead of printing a terminal summary.Why
grep '^INFO test_'inrun-tests) was brittleChanges
conftest.py--info_summary_appendfixture andpytest_terminal_summaryhook replaced by two fixtures: a session-scoped_info_summary_handler(manages alogging.FileHandler) and a function-scopedinfo_log(yields aLoggerAdapterwith the test node name). Log file is keyed offCUDA_PATHFINDER_TEST_LOAD_NVIDIA_DYNAMIC_LIB_STRICTNESSenv var so both CI runs produce separate files.info_summary_append(...)calls becomeinfo_log.info(...).ci/tools/run-tests-- Removedteepipe andgrepverification for pathfinder.upload-artifactstep (pinned v6.0.0) after the last pathfinder test run withif-no-files-found: error.Made with Cursor