Skip to content

Fix broken append and var_names paths in SU2.io.data - #2867

Open
pragyaangaur wants to merge 1 commit into
su2code:developfrom
pragyaangaur:fix-io-data-append
Open

Fix broken append and var_names paths in SU2.io.data#2867
pragyaangaur wants to merge 1 commit into
su2code:developfrom
pragyaangaur:fix-io-data-append

Conversation

@pragyaangaur

Copy link
Copy Markdown

Proposed Changes

Fixes two defects in SU2_PY/SU2/io/data.py. Both make documented public API paths of SU2.io raise immediately on any input.

1. save_data(..., append=True) raises NameError.

The append branch calls load(...), which does not exist in the module. The function is load_data(). The stale end-of-function markers (#: def load(), #: def save()) suggest the functions were renamed at some point and this internal call site was missed. git log -S dates it to the v2.0.2 import.

Simply calling load_data() here is not a correct fix. filelock is not reentrant (it is an O_CREAT | O_EXCL lock on a .lock sidecar file), and save_data makes this call from inside its own with filelock(file_name) block. The naive change therefore swaps an instant NameError for a 10 second stall followed by FileLockException, which is strictly worse.

Instead the lock-free read body is extracted into a private _read_data() helper. load_data() wraps it in the filelock, and save_data() calls it directly under the lock it already holds. This keeps the read-modify-write atomic, rather than hoisting the read outside the lock and introducing a TOCTOU race.

2. load_data(var_names=...) raises RuntimeError.

The filter deletes keys while iterating input_data.keys(). This was safe on Python 2, where .keys() returned a list copy, and has been broken since the Python 3 migration. It only triggers when at least one key is actually dropped, which is why a single-key call appears to work. Fixed by iterating a copy of the keys.

Both fixes sit in the same pair of functions and share one root cause, namely leftovers from a rename and from the Python 2 to 3 migration, so I have kept them in a single PR. Happy to split them if reviewers prefer.

Also removes a scipy import probe in load_data() that becomes dead once the read moves into the helper, and corrects the two stale marker comments.

Related Work

No open issue.

No in-tree caller currently passes append=True or var_names, so there is no regression risk to the optimization drivers. These are broken paths in a documented public API rather than a live crash in the design loop.

Out of scope, flagged for a possible follow-up: the matlab (.mat) path of save_data fails with TypeError: 'method' object does not support item assignment inside mat_bunch. I verified this behaves identically before and after this change, and did not touch it, in order to keep this PR to one thing.

Could a maintainer please add the changelog:fix label.]

Verification

SU2_PY has no Python test harness (UnitTests/ is C++/Catch2 only), so no test file is added. The following reproduces both failures on develop and passes on this branch:

import sys, os, tempfile
sys.path.insert(0, "SU2_PY")
os.environ.setdefault("SU2_RUN", tempfile.mkdtemp())
from SU2.io.data import save_data, load_data
 
os.chdir(tempfile.mkdtemp())
save_data("z.pkl", {"DRAG": 0.1, "LIFT": 0.9, "MOMENT_Z": 0.02})
save_data("z.pkl", {"CD": 1.0}, append=True)   # was NameError
print(sorted(load_data("z.pkl")))               # ['CD', 'DRAG', 'LIFT', 'MOMENT_Z']
print(load_data("z.pkl", var_names="DRAG"))     # was RuntimeError

Before and after on develop:

Call Before After
save_data(append=True) NameError OK
load_data(var_names="DRAG") RuntimeError OK
load_data(var_names=["DRAG", "LIFT"]) RuntimeError OK
load_data() plain (regression check) OK OK

pre-commit run --files SU2_PY/SU2/io/data.py passes.

PR Checklist

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with --warnlevel=3 when using meson). (N/A, Python only)
  • My contribution is commented and consistent with SU2 style (https://su2code.github.io/docs_v7/Style-Guide/).
  • I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • I have added a test case that demonstrates my contribution, if necessary. (No Python test harness exists in the repo; a runnable reproduction is included above.)
  • I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp), if necessary. (N/A, internal bug fix with no user-facing API change.)

save_data(append=True) called load(), which does not exist; the function
was renamed to load_data() and this call site was missed. Calling
load_data() directly is not sufficient because filelock is not reentrant
and save_data already holds the lock, so the read body is extracted into
a private _read_data() helper used by both.

load_data(var_names=...) deleted from the dictionary while iterating
.keys(), which raises RuntimeError on Python 3.
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.

1 participant