Reports test durations in CI #824
Conversation
There was a problem hiding this comment.
Pull request overview
Adds inline per-test duration reporting to CI logs to help identify slow tests.
Changes:
- Add a
conftest.pypytest hook that prints each test’s runtime whenPYTEST_INLINE_DURATIONS=1. - Export
PYTEST_INLINE_DURATIONS=1in CI workflow steps running unit and functional tests. - Introduce caching/regex refactors in
arc/family/family.py(note: appears unrelated to CI duration reporting).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
conftest.py |
Adds a pytest reporting hook to print per-test durations when enabled via env var. |
arc/family/family.py |
Adds global regex compilation and caches for parsed entries / reaction families. |
.github/workflows/ci.yml |
Enables duration printing in CI by exporting PYTEST_INLINE_DURATIONS=1 for pytest runs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
arc/family/family.py
Outdated
| signature = (len(groups_as_lines), groups_as_lines[0] if groups_as_lines else '', groups_as_lines[-1] if groups_as_lines else '') | ||
| cache_key = id(groups_as_lines) | ||
| cached = _ENTRIES_CACHE.get(cache_key) | ||
| if cached is not None and cached[0] == signature: |
There was a problem hiding this comment.
The new _ENTRIES_CACHE is keyed by id(groups_as_lines) with a very weak signature (len + first/last line). id() values can be reused after GC, and different lists can easily share this signature, which can return a wrong cached entries_map. Also, the cache can grow unbounded since entries are never evicted. Consider using a deterministic cache key derived from the full content (e.g., hash of ''.join(groups_as_lines)), and/or an LRU/size-bounded cache, or key the cache by a stable identifier (e.g., family label / groups file path) instead of id().
| _ENTRY_RE = re.compile(r'entry\((.*?)\)', re.DOTALL) | ||
| _ENTRY_LABEL_RE = re.compile(r'label = "(.*?)"') | ||
| _ENTRY_GROUP_RE = re.compile(r'group =(.*?)(?=\w+ =)', re.DOTALL) | ||
| _ENTRIES_CACHE: Dict[int, Tuple[Tuple[int, str, str], Dict[str, str]]] = {} | ||
| _REACTION_FAMILY_CACHE: Dict[Tuple[str, bool], 'ReactionFamily'] = {} | ||
|
|
There was a problem hiding this comment.
This PR is described as adding pytest duration reporting in CI, but this file introduces additional behavior changes (regex precompilation plus global caches and get_reaction_family() usage) that appear unrelated to CI reporting. If these are intentional, they should be called out in the PR description; otherwise, consider splitting them into a separate PR to keep scope focused and reviewable.
| key = (label, consider_arc_families) | ||
| cached = _REACTION_FAMILY_CACHE.get(key) | ||
| if cached is not None: | ||
| return cached | ||
| family = ReactionFamily(label=label, consider_arc_families=consider_arc_families) | ||
| _REACTION_FAMILY_CACHE[key] = family | ||
| return family |
There was a problem hiding this comment.
_REACTION_FAMILY_CACHE stores ReactionFamily instances globally with no eviction. These objects hold groups_as_lines and derived data, so this can retain significant memory over a long-running process and makes behavior dependent on global cache state. Consider adding a bounded/LRU cache, an explicit cache clear mechanism, or limiting caching to call sites that can guarantee reuse (instead of caching unconditionally at module scope).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
arc/processor.py
Outdated
| thermo_info_path = os.path.join(output_directory, 'thermo.info') | ||
| if not os.path.isfile(thermo_info_path): | ||
| labels = [spc.label for spc in species_to_compare] | ||
| max_label_len = max((len(label) for label in labels), default=0) | ||
| thermo_sources = '\nSources of thermodynamic properties determined by RMG for the parity plots:\n' | ||
| for spc in species_to_compare: | ||
| comment = spc.rmg_thermo.comment | ||
| thermo_sources += ' {0}: {1}{2}\n'.format(spc.label, | ||
| ' ' * (max_label_len - len(spc.label)), | ||
| comment) | ||
| with open(thermo_info_path, 'w') as f: | ||
| f.write(thermo_sources) |
There was a problem hiding this comment.
This block is effectively dead/redundant: plotter.draw_thermo_parity_plots(..., path=output_directory) already writes thermo.info unconditionally (see arc/plotter.py:458), so if not os.path.isfile(thermo_info_path) will almost never be true. Consider removing this duplicate thermo.info generation logic, or move/centralize the thermo.info formatting in the plotter if you need different behavior.
| thermo_info_path = os.path.join(output_directory, 'thermo.info') | |
| if not os.path.isfile(thermo_info_path): | |
| labels = [spc.label for spc in species_to_compare] | |
| max_label_len = max((len(label) for label in labels), default=0) | |
| thermo_sources = '\nSources of thermodynamic properties determined by RMG for the parity plots:\n' | |
| for spc in species_to_compare: | |
| comment = spc.rmg_thermo.comment | |
| thermo_sources += ' {0}: {1}{2}\n'.format(spc.label, | |
| ' ' * (max_label_len - len(spc.label)), | |
| comment) | |
| with open(thermo_info_path, 'w') as f: | |
| f.write(thermo_sources) |
| def _get_entries_map(groups_as_lines: List[str]) -> Dict[str, str]: | ||
| signature = (len(groups_as_lines), groups_as_lines[0] if groups_as_lines else '', groups_as_lines[-1] if groups_as_lines else '') | ||
| cache_key = id(groups_as_lines) | ||
| cached = _ENTRIES_CACHE.get(cache_key) | ||
| if cached is not None and cached[0] == signature: | ||
| return cached[1] | ||
| groups_str = ''.join(groups_as_lines) | ||
| entries_map: Dict[str, str] = {} |
There was a problem hiding this comment.
_get_entries_map() caches by id(groups_as_lines) and only validates via a very weak (len, first, last) signature. This can return incorrect results if a different list instance later reuses the same id (possible after GC) and happens to match the signature, and _ENTRIES_CACHE will also grow without bound over time. Prefer caching by stable content (e.g., hash/tuple of lines or the groups file path + mtime) or use a bounded/LRU cache; alternatively remove this cache since get_reaction_family() now caches ReactionFamily instances.
Adds a configuration option to display test durations directly in pytest reports. This enhances the feedback provided during testing by showing how long each test takes, helping to identify slow tests.
Improves performance by caching reaction family entries and utilizing precompiled regular expressions. Adds a cache for reaction family entries to avoid redundant file parsing and regex operations. Introduces a fingerprinting mechanism to ensure cache validity.
Introduces a worker suffix to project names in restart tests to avoid conflicts when running tests in parallel using xdist. This ensures that each worker has its own isolated project directory, preventing interference and ensuring accurate test results.
Improves the readability of inline duration reports in pytest by prefixing the duration with the test outcome (PASSED, FAILED, SKIPPED). This makes it easier to quickly identify the status of each test when durations are displayed inline.
Allows controlling conformer generation in the scissors method. This is useful for cases where conformer generation is not needed or desired, and can save computational time. Adds a generate_conformers parameter to the scissors functions to enable or disable conformer generation. If `generate_conformers` is set to None, it now checks the value of the ARC_SKIP_SCISSORS_CONFORMERS environment variable (skipping conformers if set to "1"). The ARC_SKIP_SCISSORS_CONFORMERS env var is now set temporarily for tests.
The mapping algorithm might yield slightly different, yet valid, atom maps due to the stochastic nature of conformer generation. This commit updates the tests to accept either of the two valid atom mappings. Also, temporarily changes an environment variable for testing purposes. It ensures the original environment variable value is restored after the test.
Adds reporting of test durations to the CI output, which helps in identifying slow tests and optimizing performance.
This is achieved by:
PYTEST_INLINE_DURATIONSenvironment variable in the CI workflow to enable duration reporting.conftest.pyfile that hooks into the pytest reporting mechanism to print the duration of each test.