feat: simulation search & tagging for large-scale campaigns

[maxscheurer]

Closes #23

Add user-defined metadata tags on BuildInput, discovery/store improvements (min_status passthrough), CLI search command, and interactive Textual-based TUI for browsing and filtering large simulation campaigns.

Implementation plan posted as a comment below.

[maxscheurer]

Implementation Plan

Analysis

The issue has three interconnected features: (1) tags on BuildInput, (2) discovery/store improvements, (3) interactive TUI. These build on each other — tags and discovery fixes are prerequisites for meaningful search and browsing.

Critical technical finding: BuildInput.hash uses self.model_dump_json() directly. Adding any field — even tags: None — changes the JSON output and breaks all existing hashes. The fix: self.model_dump_json(exclude={"tags"}) in the hash property. Since None fields are included by default in Pydantic v2, this exclusion is mandatory even when tags are unset.

The CSV dot-notation in prepare.py already handles arbitrary nesting via reduce(setdefault) — tags.formulation_id will become {"tags": {"formulation_id": "value"}} automatically once BuildInput accepts the field.

Deliverables

Stage 1: Tags + Discovery Fixes

• BuildInput gains tags: dict[str, str] | None = None field, excluded from hash
• SimulationStore gains min_status parameter (constructor + discover)
• Tags round-trip through CSV dot-notation and YAML
• Tags surfaced in build_metadata_table() output (merged into base columns alongside hash/path)

Stage 2: CLI Search Command

• SimulationStore.search() method with tag, type, status, SMILES, hash filters
• SMILES matching via RDKit substructure search (graceful fallback if RDKit unavailable)
• mdfactory search CLI subcommand calling store search method
• Tabular output (Rich table) with hash, type, status, tags, path

Stage 3: Textual TUI

• New [tui] optional extra with textual dependency
• mdfactory browse command launches interactive browser
• DataTable with live filtering — calls same SimulationStore.search() as CLI
• Schema-adaptive filter panels based on simulation_type
• Per-simulation detail view

Acceptance Criteria

1. BuildInput(tags={"project": "foo"}).hash == BuildInput(tags=None).hash == BuildInput().hash for same chemistry
2. Existing simulations (no tags in YAML) load without error; build_input.tags is None
3. CSV with tags.formulation_id column produces BuildInput with tags={"formulation_id": "value"}
4. SimulationStore(roots=[...], min_status="build") discovers build-only sims
5. build_metadata_table() output includes tag columns for simulations that have tags
6. mdfactory search --tag project=foo --type bilayer filters correctly
7. mdfactory search --smiles "CCO" finds sims containing ethanol (substructure match)
8. mdfactory browse launches Textual TUI with working filters
9. All new code has tests; existing tests still pass

Files to Modify

Stage 1:

File	Change
mdfactory/models/input.py	Add tags: dict[str, str] | None = None; modify hash to use model_dump_json(exclude={"tags"}); update metadata property to include tags
mdfactory/analysis/store.py	Add min_status to __init__ and pass through in discover(); merge tags into build_metadata_table() base columns
mdfactory/tests/test_models.py	Test tags field, hash stability (with/without tags), round-trip via to_data_row/from_data_row
mdfactory/tests/test_prepare.py	Test CSV dot-notation with tags.x columns
mdfactory/tests/test_simulation_store.py	Test min_status passthrough, tags in metadata table output

Stage 2:

File	Change
mdfactory/analysis/store.py	Add search() method with filter predicates (tags, type, status, SMILES, hash prefix)
mdfactory/utils/chemistry_utilities.py	Add SMILES substructure matching utility (reuses existing RDKit imports)
mdfactory/cli.py	Mount search_app sub-app; call SimulationStore.search()
mdfactory/tests/test_simulation_store.py	Test search() with various filter combinations
mdfactory/tests/test_search_cli.py	Test CLI search command integration

Stage 3:

File	Change
mdfactory/tui/__init__.py	New package with lazy Textual import
mdfactory/tui/app.py	Textual App: DataTable, filter inputs, detail panel; calls SimulationStore.search()
mdfactory/tui/styles.css	Textual CSS layout
mdfactory/cli.py	Add browse command (lazy-imports tui package)
pyproject.toml	Add tui = ["textual>=0.50"] optional extra
mdfactory/tests/test_tui.py	Test app launch, filter reactivity using textual.testing

Design Principles

• Single filter implementation: SimulationStore.search() is the one filtering API. Both CLI and TUI call it — no duplicate filter logic.
• Reuse existing infra: Tags merge into build_metadata_table() loop (no separate method); SMILES utility lives in chemistry_utilities.py (already has RDKit); CLI follows existing cyclopts sub-app pattern.
• Graceful degradation: SMILES matching checks RDKit availability at call time (conda-only dep); TUI checks textual availability. Clear error messages when missing.

Testing Approach

• Hash stability: Parametrize test with same chemistry +/- tags; assert identical hash
• Backward compat: Load YAML without tags key; assert tags is None, hash unchanged
• CSV tags: DataFrame with tags.x columns; assert BuildInput.tags == {"x": value}
• Store min_status: Fixtures with build-only sims; assert discovered with min_status="build"
• Search method: Unit test filter predicates independently; combination tests
• SMILES matching: Substructure match (ethanol in propanol), non-match, RDKit-unavailable path
• CLI search: Mock store; test output formatting and flag parsing
• TUI: textual.testing.App.run_test() for headless filter/table behavior

Risks and Open Questions

1. Tag value types: Using dict[str, str] per issue spec. Numeric CSV values stringified. Upgradable later.
2. SMILES matching: Starting with substructure (HasSubstructMatch) — most chemically useful. Tanimoto similarity deferred.
3. RDKit availability: Conda-only. Search/TUI gracefully degrade with clear error when RDKit missing.
4. Discovery caching: Deferred to follow-up issue — performance optimization, not correctness.
5. TUI scope: Read-only (browse/filter). Actions (run analysis, submit) deferred.
6. Tag immutability: Tags are set at build time. Updating tags post-build is a future enhancement.

---

Plan created by mach6

[maxscheurer]

Progress Update

Implementation of issue 23 — simulation search, tagging & TUI browser:

Delivered

• Tags on BuildInput: tags: dict[str, str] | None field, excluded from hash via model_dump_json(exclude={"tags"}) for stability
• Store improvements: min_status param on discover(), search() method with 5 filter predicates (type, status, hash_prefix, tags, smiles substructure)
• Chemistry utilities: smiles_substructure_match() with RDKit logger suppression
• CLI: mdfactory search command with Rich table output and all filter flags
• TUI: mdfactory browse interactive browser with live-filtering DataTable, Select widgets for type/status, Input for hash/tags/SMILES, detail panel with full composition info
• Tests: 68 targeted tests covering models, store, search, CLI, TUI, and chemistry utilities

Not included

• Molecule structure viewer (deferred — terminal image rendering over SSH was insufficient)

Commit: 5c0786b

---

Progress tracked by mach6

[maxscheurer]

Code Review

Critical

No critical findings.

Important

Finding 1 — Infinite poll loop when a Parsl future never completes (error-auditor, confidence: 85)
mdfactory/orchestration/build.py — The while True poll loop exits only when done_count == total. If a Parsl future never transitions to done() (e.g. SLURM job OOM-killed before Parsl detects it, or Parsl's monitoring thread crashes), the loop spins forever. User sees progress frozen with no error. Ctrl+C works, but there's no automatic timeout/give-up path.

Finding 2 — Schema-specific filters not implemented (completeness-checker, confidence: 95)
Neither CLI mdfactory search nor the TUI exposes per-type filter fields (e.g. z_padding for bilayer, target_density for mixedbox). The --type flag only filters by simulation_type string. Issue AC7 explicitly requires adaptive schema-specific filter panels.

Finding 3 — SimulationStore.search(smiles=...) is completely untested (test-reviewer, confidence: 100)
The SMILES substructure filter (iterates species, calls smiles_substructure_match, guards missing RDKit) has zero test coverage. test_chemistry_search.py covers the utility in isolation, but the integration path through search() is never exercised.

Finding 4 — SimulationStore.search(status=...) filter is completely untested (test-reviewer, confidence: 100)
The status parameter filter uses STATUS_ORDER index arithmetic. No test controls .status on mock Simulation objects to verify the threshold logic.

Finding 5 — Chemistry extractor functions and SimulationStore convenience methods are untested (test-reviewer, confidence: 100)
Six public SimulationStore methods (build_lnp_chemistry_table, build_all_species_table, build_chemistry_table, load_analysis_with_lnp_chemistry, load_analysis_with_all_species, load_analysis_with_chemistry) and three extractor functions (extract_all_species, make_chemistry_extractor, get_chemistry_extractor) have zero tests.

Suggestions

Finding 6 — @work(thread=True) without exclusive=True causes stale-results race (code-reviewer, confidence: 90)
mdfactory/tui/app.py line 242 — Every keystroke launches a new background worker. If a slow search finishes after a fast one, it overwrites the cache with stale results. Fix: @work(thread=True, exclusive=True).

Finding 7 — CLI search doesn't catch ValueError for invalid SMILES (code-reviewer, confidence: 85)
mdfactory/cli.py — store.search(smiles=smiles) raises ValueError on invalid SMILES, producing a raw traceback. The TUI correctly catches this with except (ValueError, ImportError).

Finding 8 — Preprocessing subprocess has no timeout (error-auditor, confidence: 90)
mdfactory/cli.py analysis_preprocess — subprocess.run(cmd, ...) has no timeout. A stuck script blocks all subsequent preprocessing sequentially with no diagnostic output.

Finding 9 — _scancel_jobs swallows unexpected failures at DEBUG level (error-auditor, confidence: 82)
mdfactory/orchestration/session.py — Both non-zero scancel return codes and unexpected exceptions (PermissionError, OSError) are logged at DEBUG. SLURM jobs can remain running after Python exits with no visible indication.

Finding 10 — Documentation and examples for tagging workflow absent (completeness-checker, confidence: 95)
Issue AC8 requires documentation. No docs cover tags.key CSV notation, mdfactory search, or mdfactory browse.

Finding 11 — TUI _run_search silently drops malformed tag tokens (test-reviewer, confidence: 90)
mdfactory/tui/app.py — Tag tokens without = are silently ignored (no filter applied, no user feedback). CLI raises SystemExit(1) for the same input. Behavior divergence is untested.

Finding 12 — SimulationDetail.update_detail() never tested with real data (test-reviewer, confidence: 85)
The detail panel rendering path (rich formatting, species iteration, try/except) is completely unexercised in tests.

Finding 13 — Three-way duplicate file-output sink in CLI pull commands (simplifier, confidence: 95)
mdfactory/cli.py — Identical 8-line JSON/CSV write block appears in sync_pull_systems, sync_pull_analysis, sync_pull_artifacts. Extract to _write_df_to_file() helper.

Finding 14 — Two-way duplicate SLURM config resolution block (simplifier, confidence: 93)
mdfactory/cli.py — Identical 30-line autodiscovery-or-manual SLURM config block appears in analysis_run and analysis_artifacts_run. Extract to _resolve_slurm_config().

Finding 15 — TOCTOU race on _results_cache in TUI detail panel (error-auditor, confidence: 82)
mdfactory/tui/app.py — Background thread replaces self._results_cache while main thread reads len() + index. Fix: capture cache = self._results_cache into a local before the check.

Finding 16 — Select.BLANK guard is dead code (code-reviewer, confidence: 90)
mdfactory/tui/app.py — allow_blank=False + option values mean is not Select.BLANK is always True. The else branch is unreachable.

Finding 17 — Imports inside per-row loop in search() (code-reviewer, confidence: 90)
mdfactory/analysis/store.py — STATUS_ORDER and smiles_substructure_match imports are inside the loop. The ImportError for RDKit is raised mid-iteration rather than fail-fast before the loop.

Finding 18 — remove_all_analyses(simulation_type=...) filter path untested (test-reviewer, confidence: 90)
The simulation_type filter param is tested for remove_all_artifacts but not for the analogous remove_all_analyses.

Finding 19 — Additional simplification opportunities in CLI (simplifier, confidence: 85–92)
Push-results logging (2 sites), show-and-confirm removal pattern (2 sites), parallel-dispatch block (2 sites), and tags_str formatting (3 sites across 2 files) are all duplicated.

Strengths

• Clean architecture: Single SimulationStore.search() method serves both CLI and TUI — no duplicated filter logic
• Robust Parsl lifecycle: parsl_session context manager with proper DFK guard, cleanup, and scancel on exit
• Comprehensive cluster autodiscovery: Well-structured parsing of sinfo/sacctmgr with good error handling
• Hash stability: Tags correctly excluded from hash computation; backward-compatible with existing simulations
• Strong test count: 68+ new tests covering models, store, CLI, TUI, orchestration, and cluster
• Good lazy-import pattern: Optional deps (Parsl, Textual, RDKit) gracefully degrade with clear messages

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[maxscheurer]

Review Assessment

Review comment

Classifications

Finding	Classification	Reasoning
1: Infinite poll loop when Parsl future never completes	genuine	Confirmed: while True loop in _wait_with_progress() exits only when done_count == total. No timeout or stall detection. Important for production use, though KeyboardInterrupt works.
2: Schema-specific filters not implemented (AC7)	deferred	Real gap vs acceptance criteria, but a substantial UI feature. Track as follow-up issue.
3: search(smiles=...) untested	genuine	Confirmed: no test exercises the SMILES parameter path through search(). Integration with species iteration and ImportError handling is new code.
4: search(status=...) untested	genuine	Confirmed: no test passes a status parameter to search(). The STATUS_ORDER threshold logic is non-trivial.
5: Chemistry extractors and convenience methods untested	genuine	Confirmed: six public SimulationStore methods and three extractor functions have zero test coverage, all new code in this PR.
6: @work without exclusive=True race	genuine	Confirmed: each keystroke spawns a new worker without cancelling the previous. Slow search completing after fast one overwrites cache with stale data.
7: CLI search doesn't catch ValueError for invalid SMILES	genuine	Confirmed: search_simulations() calls store.search(smiles=smiles) with no try/except. TUI handles this correctly. Users get raw traceback.
8: Preprocessing subprocess has no timeout	nitpick	By design — user scripts have unknown execution time. A configurable timeout would be nice but isn't a correctness issue.
9: _scancel_jobs swallows at DEBUG level	nitpick	Reasonable — scancel often fails for benign reasons (job already finished). The TimeoutExpired case does log at WARNING appropriately.
10: Documentation absent (AC8)	deferred	Real gap vs AC8. No docs directory exists. Track as separate documentation task.
11: TUI silently drops malformed tag tokens	nitpick	Reasonable for live-filtering — user is mid-keystroke typing project=foo. Showing errors on partial input would be worse UX.
12: SimulationDetail.update_detail() never tested	nitpick	TUI rendering code is inherently hard to unit-test. Pure display formatter with no complex logic.
13: Three-way duplicate file-output sink	genuine	Confirmed: identical 8-line write block at three sites in cli.py. Should extract to helper.
14: Two-way duplicate SLURM config resolution	genuine	Confirmed: identical ~30-line block in analysis_run and analysis_artifacts_run. Should extract to helper.
15: TOCTOU race on _results_cache	false positive	Same root cause as finding 6. Fixing @work(exclusive=True) eliminates the race — only one worker active at a time. Not a separate issue.
16: Select.BLANK guard is dead code	genuine	With allow_blank=False and None as the "All" sentinel, is not Select.BLANK is always True. Guards are misleading dead code.
17: Imports inside per-row loop in search()	genuine	ImportError for missing RDKit fires mid-iteration rather than upfront. Should hoist imports above the loop for fail-fast behavior.
18: remove_all_analyses filter path untested	genuine	Confirmed: simulation_type filter is tested for artifacts but not analyses. Low-priority gap.
19: Additional CLI duplication	nitpick	Minor patterns, not worth refactoring in this PR.

Action Plan

1. Fix @work(thread=True) race in TUI — add exclusive=True to _run_search (finding 6, also resolves finding 15)
2. Hoist imports out of per-row loop in search() — fail-fast on missing RDKit (finding 17)
3. Add ValueError handling in CLI search command for invalid SMILES (finding 7)
4. Remove dead Select.BLANK guards in TUI (finding 16)
5. Add tests for search(smiles=...) integration (finding 3)
6. Add tests for search(status=...) threshold logic (finding 4)
7. Add tests for chemistry extractors (finding 5)
8. Extract duplicate file-output helper in CLI (finding 13)
9. Extract duplicate SLURM config resolution in CLI (finding 14)
10. Add timeout/stall detection to Parsl poll loop (finding 1)
11. Add test for remove_all_analyses(simulation_type=...) filter (finding 18)

Deferred (track separately):

• Finding 2: Schema-specific filters (AC7) — separate issue needed
• Finding 10: Documentation for tagging workflow (AC8) — separate issue needed

---

Assessment by mach6

[maxscheurer]

Progress Update

Addressed 11 review findings from the mach6-review:

Bug fixes

• Finding 1: Added stall_timeout (600s default) to Parsl poll loop — warns when no futures complete for extended period
• Finding 6: Added exclusive=True to TUI @work(thread=True) — prevents stale search results race
• Finding 7: Added try/except (ValueError, ImportError) in CLI search command for invalid SMILES
• Finding 16: Removed dead Select.BLANK guards in TUI (always True due to allow_blank=False)
• Finding 17: Hoisted STATUS_ORDER and smiles_substructure_match imports above the per-row loop for fail-fast behavior

Refactoring

• Finding 13: Extracted _write_output() helper — replaced 3 identical file-output blocks in CLI
• Finding 14: Extracted _resolve_slurm_config() helper — replaced 2 identical SLURM config blocks in CLI

Tests added (19 new tests)

• Finding 3: 4 tests for search(smiles=...) integration (match, no-match, filtering, ImportError)
• Finding 4: 3 tests for search(status=...) threshold logic (includes, excludes, invalid raises)
• Finding 5: 11 tests for chemistry extractors (extract_all_species, get_chemistry_extractor, build_all_species_table, build_chemistry_table) in new test_chemistry_extractors.py
• Finding 18: 1 test for remove_all_analyses(simulation_type=...) filter path

Commit: 8437382

---

Progress tracked by mach6