feat: include detected GROMACS modules in default worker init for simulate

[gregorweiss]

Summary

Closes #35.

When the TUI is invoked for simulate (i.e. stages is non-empty or None), _default_worker_init() now detects loaded GROMACS modules from $LOADEDMODULES and prepends module load commands to the default worker init suggestion.

This ensures that when a user has loaded GROMACS via module load gromacs/... on the login node, the same module is suggested for compute node activation — so gmx/gmx_mpi are available on workers.

Changes

• _detect_gromacs_modules() — new helper that parses $LOADEDMODULES for modules with gromacs or gmx prefix (case-insensitive)
• _default_worker_init(for_simulate=False) — added for_simulate kwarg; when True, prepends detected GROMACS module loads separated by ;
• Both call sites (_configure_with_cluster, _configure_manual) now pass for_simulate=True when stages indicate simulate context
• 11 new tests covering detection logic and integration with the worker init builder

Example output

With LOADEDMODULES=cuda/12.2:gromacs/2024.3-gpu:openmpi/4.1 and pixi environment present:

module load gromacs/2024.3-gpu; eval "$(pixi shell-hook --manifest-path /path/to/project -e default)"

For build context (empty stages), only the pixi shell-hook is included — no module load.

[gregorweiss]

Code Review

Critical

Finding 1 (test-reviewer): TestPromptStageOverrides — all 8 unit tests fail with TypeError (pre-existing)

• File: mdfactory/tests/test_orchestration_tui.py — TestPromptStageOverrides class
• Confidence: 100
• Every test calls _prompt_stage_overrides(common_cpus=…, common_gres=…, common_gmx=…) without the required stages positional argument (made required in commit 836d3dd). All 8 fail immediately with TypeError. Pre-existing on the base branch but not repaired in this PR which adds 84 new lines to the same file.

Important

Finding 2 (test-reviewer): Integration path for build context (stages=()) is not tested

• File: mdfactory/orchestration/tui.py — both call sites
• Confidence: 95
• The routing expression for_simulate=stages is None or len(stages) > 0 evaluates to False only when stages=() (build context). All existing integration tests call configure_slurm_interactive() without stages, always exercising for_simulate=True. No test verifies the build-context path routes to for_simulate=False.

Suggestions

Finding 3 (test-reviewer): Combined GROMACS modules + pixi env present is not tested

• File: mdfactory/tests/test_orchestration_tui.py — TestDefaultWorkerInit
• Confidence: 85
• Both test_with_simulate_includes_modules and test_with_simulate_multiple_modules patch Path to suppress pixi detection. The combined output (module load gromacs/…; eval "$(pixi shell-hook …)") — the primary production use case — is never asserted.

Finding 4 (code-reviewer): import os placed inside function body instead of module level

• File: mdfactory/orchestration/tui.py (line 69 in _detect_gromacs_modules)
• Confidence: 88
• os is a standard library module with no side effects or optional-dependency status. All other stdlib imports in this file are at module level. The lazy-import pattern is only used for questionary (optional extra) and mdfactory (circular-import avoidance). Neither reason applies to os.

Strengths

• Clean separation of concerns: _detect_gromacs_modules() is a pure, testable helper
• Correct for_simulate routing logic at both call sites
• _detect_gromacs_modules() is infallible by construction (env-var read → split → filter)
• Good test coverage for the new detection logic (7 tests for _detect_gromacs_modules, 4 for _default_worker_init)
• All 3 acceptance criteria from issue 35 are fully met

Agents run: code-reviewer, error-auditor (no findings), test-reviewer, completeness-checker (all AC met)

---

Reviewed by mach6

[gregorweiss]

Review Assessment

#37 (comment)

Classifications

Finding	Classification	Reasoning
1: TestPromptStageOverrides tests fail with TypeError	Deferred	Factual: TRUE — _prompt_stage_overrides requires 4 positional args since commit 836d3dd, but all 8 test calls omit stages. Scope: Pre-existing defect on the base branch (feat/parsl-simulate), not introduced by this PR. Issue 35 scope is GROMACS module detection, not repairing unrelated stage-override tests.
2: Integration path for build context (stages=()) not tested	Deferred	Factual: TRUE — no integration test passes stages=() to exercise for_simulate=False. Scope: The for_simulate=False path IS unit-tested directly (test_without_simulate_no_modules). The routing expression is a single trivial boolean. Both branches proven correct at the unit level. Not a blocking gap.
3: Combined GROMACS modules + pixi env not tested	Deferred	Factual: TRUE — tests patch Path to suppress pixi detection, so combined output never asserted. Scope: Implementation is trivially correct: two independent list appends joined by "; ".join(parts). Both paths independently verified. Join logic has no conditional branching that could fail. Low risk.
4: import os inside function body	Nitpick	Factual: TRUE — os imported at function scope while other stdlib imports are at module level. Scope: Pure style preference. Functionally correct. No impact on authorized scope.

Action Plan

No genuine issues — PR is clear to merge.

Deferred items for optional follow-up (not blocking):

1. Fix TestPromptStageOverrides by adding the required stages argument (pre-existing bug from 836d3dd on base branch)
2. Add integration test calling configure_slurm_interactive(stages=()) to verify build-context routing
3. Add combined GROMACS + pixi test asserting the semicolon-separated output

---

Assessment by mach6