Skip to content

[codex] Fix GX eik input handling and subprocess errors#2143

Draft
rmchurch wants to merge 1 commit into
PlasmaControl:yge/gxfrom
rmchurch:gx
Draft

[codex] Fix GX eik input handling and subprocess errors#2143
rmchurch wants to merge 1 commit into
PlasmaControl:yge/gxfrom
rmchurch:gx

Conversation

@rmchurch

Copy link
Copy Markdown

What changed

  • force generated GX inputs to use geo_option = "eik" so DESC's plain-text geometry files match the GX input mode
  • wrap GX subprocess failures and timeouts in readable runtime errors that include stdout.gx / stderr.gx context
  • add targeted tests for input rewriting and subprocess error reporting

Why

The GX wrapper was generating a plain-text geometry file while allowing templates to keep geo_option = "nc". That made GX fail with an opaque nonzero exit, and the callback path surfaced it as a raw subprocess failure instead of a useful GX-specific error.

Impact

This makes the GX integration robust against NC-based templates and preserves actionable failure context when GX exits nonzero.

Validation

  • /global/cfs/projectdirs/m4505/rchurchi/conda-envs/desc-gx/bin/python -m pytest tests/test_external_gx.py -q

@YigitElma YigitElma self-requested a review March 29, 2026 19:21

@YigitElma YigitElma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for this @rmchurch! I was naively expecting the user to put eik, but I guess people use the same template file for multiple cases.

If this is ready, I can approve/merge it to yge/gx, and create a GX wrapper PR later. Does this sound good to you?

Comment thread tests/test_external_gx.py

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We actually don't require writing tests for the desc.external module. But these look good to me. We can decide to keep them or not in the actual PR for the yge/gx branch.

@YigitElma YigitElma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @rmchurch, if you can resolve the merge conflicts, I can merge this to the other branch and finish it up on the other branch.

Comment thread desc/external/gx.py
flags=re.MULTILINE,
)
if geo_option_subs == 0:
data, geo_option_subs = re.subn(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In the other branch, I switched to using a loop to update the file content. This is mainly because regex operations are very hard to read, and some other developers wanted that.

@rmchurch Can you switch to the other method when resolving the merge conflicts?

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