Skip to content

Fix relative path resolution in tool file readers - #142

Merged
keceli merged 2 commits into
argonne-lcf:devfrom
reowszer:dev-fix-path-resolution
Jul 19, 2026
Merged

Fix relative path resolution in tool file readers#142
keceli merged 2 commits into
argonne-lcf:devfrom
reowszer:dev-fix-path-resolution

Conversation

@reowszer

@reowszer reowszer commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Problem

File writers (smiles_to_coordinate_file, run_ase output) route relative
paths through _resolve_path into CHEMGRAPH_LOG_DIR. File readers didn't —
they opened the raw path from cwd. An agent that writes water.xyz then reads
it by bare name gets FileNotFoundError.

Seen with meta-llama/Llama-3.3-70B-Instruct on ALCF: it passes bare filenames
between tools. Models that pass absolute paths (e.g. gpt-oss-120b) avoid it.

Fix

Add _resolve_existing_path(); apply in run_ase, extract_output_json,
file_to_atomsdata, generate_html. Returns the path if it exists, else the
log-dir location, else raw (missing-file errors still surface). Absolute and
cwd-relative paths unchanged.

Tests

  • tests/test_ase_input_path_resolution.py (EMT, no network): 5 pass.
  • Full suite: 264 passed, 28 skipped. 3 failures in test_human_interrupt.py
    are pre-existing on dev (b10180a), unrelated.

Writers (smiles_to_coordinate_file, run_ase output) route relative paths
through _resolve_path into CHEMGRAPH_LOG_DIR, but readers opened them from
cwd -> FileNotFoundError on bare names like "water.xyz".

Add _resolve_existing_path() and use it in run_ase, extract_output_json,
file_to_atomsdata, generate_html. Prefers an existing path, else the
log-dir location, else raw (so missing-file errors still surface).

Add regression tests (EMT, no network).
@tdpham2

tdpham2 commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Hi @reowszer. Thanks for PR. I think this is more an LLM issue (with generating the right tool call parameters, e.g file path), than an actual tool issue (e.g reproducible bugs even with frontier models on Argo).

All the tools should return the absolute path to the file that was written. A good model should be able to pick up the full path. For example, in smiles_to_coordinate_file, the tool returns the absolute path. Even though the agent may specify water.xyz, it should still know that the file written isn't ./water.xyz, but /path/to/water.xyz.

    return {
        "ok": True,
        "artifact": "coordinate_file",
        "path": os.path.abspath(final_output_file),
        "smiles": smiles,
        "natoms": len(numbers),
    }

@tdpham2

tdpham2 commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Closing this. Feel free to reopen if any issues pop up!

@tdpham2 tdpham2 closed this Jul 10, 2026
@tdpham2 tdpham2 reopened this Jul 10, 2026
@tdpham2

tdpham2 commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

@reowszer I reopened since I think this will help long-term for smaller models. Could you please help me check the related MCP tools current under dev and see if you can add a similar fix?

@tdpham2
tdpham2 self-requested a review July 14, 2026 18:22
@tdpham2 tdpham2 added the enhancement New feature or request label Jul 14, 2026
@reowszer
reowszer force-pushed the dev-fix-path-resolution branch from b7e47cb to 3ff2c52 Compare July 15, 2026 18:41
Extends the CHEMGRAPH_LOG_DIR path resolution from the LangChain tool
readers to the MCP servers and the graspa/xanes cores, so a small model
that echoes back a bare filename ("water.xyz") for a file a sibling tool
wrote into the log dir still resolves it instead of failing with
FileNotFoundError.

- execution/utils.resolve_structure_files: resolve each listed filename
  against the log dir (shared by the ase/mace/graspa/xanes _hpc ensemble
  tools). Directory inputs are left unchanged.
- mcp/ase_mcp_hpc.py, mcp/mace_mcp_hpc.py: _embed_inline_if_local resolves
  the bare name on the submitting host before its local-vs-remote check and
  stores the resolved absolute path back into the job. Worker code unchanged.
- tools/graspa_core.py, tools/xanes_core.py: resolve input_structure_file via
  _resolve_existing_path instead of Path(...).resolve(), so the single-
  structure _hpc path (which delegates here) also resolves bare names. The
  ase/mace cores already did this via run_ase_core.
- mcp/data_analysis_mcp.py: aggregate_simulation_results resolves each input.
- mcp/hpc_misc_mcp.py: inspect_json checks the log dir before its nearby-files
  fallback.
- mcp/graspa_mcp_parsl.py, mcp/xanes_mcp_parsl.py: same fallback in their
  copied list-resolution logic (deprecated servers; included for consistency).

The general server (mcp/mcp_tools.py) already delegates to the fixed core and
returns absolute paths; added tests to pin that behavior.

The _hpc worker functions are deliberately left untouched: when the backend
shares the filesystem the worker delegates to the (now-fixed) cores, and when
it does not the bare name is resolved on the submitting host before submission.
@reowszer
reowszer force-pushed the dev-fix-path-resolution branch from 3ff2c52 to 68c434a Compare July 15, 2026 19:22
@reowszer

Copy link
Copy Markdown
Contributor Author

The MCP tools have been checked, and the same _resolve_existing_path fallback added across their file readers (the _hpc and data_analysis/hpc_misc servers). graspa_core.py and xanes_core.py were also switched from Path(...).resolve() to the fallback so the single-structure _hpc path is covered; mcp_tools.py already inherited it by delegating to the patched core. The _hpc workers were left unchanged, since bare names are resolved before submission or by the now-fixed cores.

Two things worth a look: the fallback was also applied to the deprecated graspa_mcp_parsl.py / xanes_mcp_parsl.py for consistency, which can be reverted if deprecated code should stay untouched; and the three failing test_human_interrupt.py tests already fail on dev (from b10180a), so they look unrelated.

ruff passes; tests were added for the MCP and graspa bare-name paths.

@keceli
keceli merged commit 8a24ddd into argonne-lcf:dev Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants