Skip to content

Fix: CI tests#13

Merged
hariharan-devarajan merged 5 commits intollnl:mainfrom
izzet:fix/ci-tests
Jul 12, 2025
Merged

Fix: CI tests#13
hariharan-devarajan merged 5 commits intollnl:mainfrom
izzet:fix/ci-tests

Conversation

@izzet
Copy link
Collaborator

@izzet izzet commented Jul 11, 2025

This pull request introduces several updates to the CI workflow, Python environment setup, and codebase improvements, including support for Python 3.11, enhanced virtual environment management, and new functionality for shutting down resources in the dfanalyzer module. Below is a categorized summary of the most important changes:

CI Workflow and Python Environment Updates:

  • Updated the CI matrix to test Python versions 3.9, 3.10, and 3.11.5, and adjusted the requires-python field in pyproject.toml to support Python versions >3.8, <3.12. [1] [2]
  • Added virtual environment creation and activation steps in the CI workflow to ensure isolated environments for dependency installation and testing.
  • Ensured the virtual environment is activated before installing dependencies, running tests, and executing the dfanalyzer tool. [1] [2] [3]

Codebase Enhancements:

  • Introduced a shutdown method in the dfanalyzer module to cleanly close the Dask client and cluster, improving resource management.
  • Added a docstring to the analyze_trace method in the dfanalyzer module to clarify its purpose.

Testing Improvements:

  • Updated _test_e2e in tests/test_main.py to include a call to the new shutdown method, ensuring proper cleanup of resources after tests.
  • Standardized string formatting in _test_e2e to use double quotes for consistency.

…g it for dependency installation and testing
@izzet izzet self-assigned this Jul 11, 2025
@izzet
Copy link
Collaborator Author

izzet commented Jul 11, 2025

@hariharan-devarajan pinned Python to 3.11.5 for now - Dask breaks on higher versions and I don't have time to debug it at the moment.

@izzet izzet requested a review from Copilot July 11, 2025 20:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the CI setup to cover Python 3.11, strengthens isolation with virtual environments, and improves resource management in the dfanalyzer module.

  • Updated CI matrix for Python 3.9, 3.10, and 3.11.5, added venv creation/activation, and reordered C++ test steps
  • Bumped requires-python in pyproject.toml to >3.8, <3.12 and added Python 3.11 classifier
  • Introduced a shutdown method and added a docstring in dfanalyzer/__init__.py
  • Standardized quotes in tests, re-enabled shutdown in _test_e2e, and removed two commented assertions

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.

File Description
.github/workflows/ci.yml Added Python 3.11 to the matrix, venv creation/activation steps, and moved C++ tests later
pyproject.toml Changed requires-python specifier and added Programming Language :: Python :: 3.11
dfanalyzer/init.py Added a docstring to analyze_trace and implemented a new shutdown method
tests/test_main.py Switched to double quotes, removed two commented-out assertions, and added dfa.shutdown()
Comments suppressed due to low confidence (4)

tests/test_main.py:119

  • Consider adding assertions after shutdown() to verify the Dask client and cluster are closed, ensuring the new cleanup code is actually tested.
    dfa.shutdown()

pyproject.toml:11

  • Using >3.8 excludes Python 3.8.x entirely; if you intend to keep 3.8 support, switch back to >=3.8, <3.12.
requires-python = ">3.8, <3.12"

.github/workflows/ci.yml:42

  • [nitpick] Since actions/setup-python already provides an isolated tool cache, you can skip manual venv setup or combine the venv activation and install steps into a single run block to reduce repetition.
      - name: Create and activate virtual environment

tests/test_main.py:98

  • Remove or restore these commented-out assertions to avoid dead code; if the run directory settings are still important, re-enable them as active checks.
    )

Copy link
Member

@hariharan-devarajan hariharan-devarajan left a comment

Choose a reason for hiding this comment

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

Looks good

@hariharan-devarajan hariharan-devarajan merged commit 1f5da08 into llnl:main Jul 12, 2025
3 checks passed
@izzet izzet deleted the fix/ci-tests branch July 12, 2025 19:06
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.

3 participants