Fix broken append and var_names paths in SU2.io.data - #2867
Open
pragyaangaur wants to merge 1 commit into
Open
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Fixes two defects in
SU2_PY/SU2/io/data.py. Both make documented public API paths ofSU2.ioraise immediately on any input.1.
save_data(..., append=True)raisesNameError.The append branch calls
load(...), which does not exist in the module. The function isload_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 -Sdates it to the v2.0.2 import.Simply calling
load_data()here is not a correct fix.filelockis not reentrant (it is anO_CREAT | O_EXCLlock on a.locksidecar file), andsave_datamakes this call from inside its ownwith filelock(file_name)block. The naive change therefore swaps an instantNameErrorfor a 10 second stall followed byFileLockException, 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, andsave_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=...)raisesRuntimeError.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
scipyimport probe inload_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=Trueorvar_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 ofsave_datafails withTypeError: 'method' object does not support item assignmentinsidemat_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:fixlabel.]Verification
SU2_PYhas no Python test harness (UnitTests/is C++/Catch2 only), so no test file is added. The following reproduces both failures ondevelopand passes on this branch:Before and after on
develop:save_data(append=True)NameErrorload_data(var_names="DRAG")RuntimeErrorload_data(var_names=["DRAG", "LIFT"])RuntimeErrorload_data()plain (regression check)pre-commit run --files SU2_PY/SU2/io/data.pypasses.PR Checklist
pre-commit run --allto format old commits.