Skip to content

Improve multi-machine test script configuration and documentation (#624) - #640

Merged
wshlavacek merged 2 commits into
lanl:mainfrom
psamuelvijay:fix/multi-machine-test-scripts
Aug 22, 2026
Merged

Improve multi-machine test script configuration and documentation (#624)#640
wshlavacek merged 2 commits into
lanl:mainfrom
psamuelvijay:fix/multi-machine-test-scripts

Conversation

@psamuelvijay

Copy link
Copy Markdown
Contributor
  • Replace hardcoded Python environment paths with clear edit instructions
  • Make resource configuration (nodes, CPUs, time) easy to find and modify
  • Extract hardcoded worker counts to variables in cluster_manual.sh
  • Add comprehensive README.md in tests/full_tests/ explaining:
    • How to run tests locally vs on cluster
    • How to configure each script
    • Resource requirements
    • How to interpret results and reference output
    • Troubleshooting common issues
  • Update CONTRIBUTING.md with multi-machine testing guidelines
  • Document when cluster tests should be run

The maintainer already fixed dask command names (dask-scheduler -> dask scheduler) in a recent commit. This PR addresses the remaining configuration and documentation issues from the original issue.

…nl#624)

- Replace hardcoded Python environment paths with clear edit instructions
- Make resource configuration (nodes, CPUs, time) easy to find and modify
- Extract hardcoded worker counts to variables in cluster_manual.sh
- Add comprehensive README.md in tests/full_tests/ explaining:
  - How to run tests locally vs on cluster
  - How to configure each script
  - Resource requirements
  - How to interpret results and reference output
  - Troubleshooting common issues
- Update CONTRIBUTING.md with multi-machine testing guidelines
- Document when cluster tests should be run

The maintainer already fixed dask command names (dask-scheduler -> dask scheduler)
in a recent commit. This PR addresses the remaining configuration and
documentation issues from the original issue.

Signed-off-by: psamuelvijay <samuelvijay2004@gmail.com>
@psamuelvijay

Copy link
Copy Markdown
Contributor Author

This PR addresses all the configuration and documentation issues from #624 that don't require cluster access:

✅ Fixed hardcoded Python environment paths
✅ Made resource configuration easy to find and change
✅ Added comprehensive documentation (README + CONTRIBUTING.md)
✅ Explained what to do with historical reference output

The dask command names were already fixed by the maintainers in a recent commit.

Generating fresh reference output would require running the ~30 minute test suite on an actual 2-node SLURM cluster, which I don't have access to. The README now clearly explains that the existing reference output is from 2019 and describes what users should check instead.

wshlavacek pushed a commit to psamuelvijay/PyBNF that referenced this pull request Aug 22, 2026
Fixes points found in review of lanl#640, so the docs match this repo:
- README: T6 is model checking (fit_type = check), not "configuration checking"
- README: name the per-mode summary files (test_summary_ssh.txt / _sf.txt)
- README: the project is uv-managed (uv sync); there is no requirements.txt
- README: distributed floor is >=2024.1.0 (per pyproject); the standalone
  dask-scheduler/dask-worker programs stopped installing in 2026.6.0 (per CHANGELOG)
- cluster.sh / cluster_manual.sh: replace the author-specific activate paths with
  a generic placeholder, so a run-as-written no longer sources a real user path

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
While reviewing pull request 640 we found several statements in the new
documentation that do not match this repository. This commit corrects them.

The README described test T6 as configuration checking. T6 runs the model
checking job type, which its configuration file selects with the setting
fit_type = check.

The README listed only test_summary.txt as the output file. The run_all.py
script writes test_summary.txt for a local run, test_summary_ssh.txt for the
ssh mode, and test_summary_sf.txt for the sf mode.

The README told readers to install dependencies from a requirements.txt file.
This project has no such file. It is managed with uv and a populated uv.lock
file, so dependencies are installed with uv sync.

The README said the distributed package must be version 2021.0.0 or newer. The
project requires version 2024.1.0 or newer, as set in pyproject.toml. The older
standalone programs named dask-scheduler and dask-worker stopped installing in
distributed version 2026.6.0, as noted in CHANGELOG.md.

The two cluster batch scripts still pointed at the original author's personal
environment paths. This commit replaces them with a generic placeholder path so
that running a script as written no longer tries to activate a real user's
environment.
@wshlavacek
wshlavacek force-pushed the fix/multi-machine-test-scripts branch from 012796b to 4aab9bd Compare August 22, 2026 17:02
@wshlavacek
wshlavacek merged commit 65e48b9 into lanl:main Aug 22, 2026
7 checks passed
@wshlavacek

Copy link
Copy Markdown
Collaborator

Thanks for this. The new README and the clearer cluster scripts are a real
improvement, and it is merged now.

During review I made a few small edits to the documentation so it matches the
current project setup. For example, the project uses uv and a uv.lock file
rather than a requirements file, and the distributed package floor is 2024.1.0.
I also swapped the personal environment paths in the two scripts for a generic
placeholder.

Thanks again for taking the time to send this. It is appreciated.

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.

2 participants