Ensure vs30 values are written to disk and verified before exit#540
Ensure vs30 values are written to disk and verified before exit#540AndrewRidden-Harper wants to merge 1 commit intomasterfrom
Conversation
Add flush and fsync after writing vs30 values to prevent incomplete writes when MPI terminates. Add post-completion validation on master rank to check file size and detect zero vs30 headers.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where Vs30 values were sometimes missing from output files, leading to crashes in downstream processing. It enhances data integrity by ensuring that Vs30 values are completely written to disk and then validated immediately after the simulation completes, thereby preventing data corruption and improving the robustness of the workflow. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where Vs30 values were sometimes missing from the output file. The changes introduce flush and fsync calls to ensure data is written to disk after each station is processed. Additionally, a verification step is added at the end of the simulation on the master process to check for any missing Vs30 values in the output file, aborting if any are found. The changes are logical and directly address the problem. I have one suggestion to slightly refactor the new verification code for better efficiency.
| with open(args.out_file, "r+b") as hff: | ||
| os.fsync(hff.fileno()) | ||
| with open(args.out_file, "rb") as hff: | ||
| hff.seek(HEAD_SIZE) | ||
| vs_values = np.fromfile( | ||
| hff, | ||
| count=stations.size, | ||
| dtype={ | ||
| "names": ["vs"], | ||
| "formats": ["f4"], | ||
| "offsets": [HEAD_STAT - FLOAT_SIZE], | ||
| "itemsize": HEAD_STAT, | ||
| }, | ||
| )["vs"] |
There was a problem hiding this comment.
For efficiency, you can combine these two with open(...) blocks. Opening the file in r+b mode allows for both calling os.fsync() and then reading from the file without needing to close and reopen it.
with open(args.out_file, "r+b") as hff:
os.fsync(hff.fileno())
hff.seek(HEAD_SIZE)
vs_values = np.fromfile(
hff,
count=stations.size,
dtype={
"names": ["vs"],
"formats": ["f4"],
"offsets": [HEAD_STAT - FLOAT_SIZE],
"itemsize": HEAD_STAT,
},
)["vs"]
sungeunbae
left a comment
sungeunbae
left a comment
There was a problem hiding this comment.
I am a little confused here - do you mean HF.bin output file?
| out.seek(HEAD_STAT - 2 * FLOAT_SIZE, 1) | ||
| e_dist[i].tofile(out) | ||
| vs.tofile(out) | ||
| # Ensure vs30 values are fully written to disk before MPI terminates the process |
|
I just discovered a problem with the extra validation check I put in hf_sim.py. I've converted this back to draft until my fix is confirmed, then I will include it and mark is as ready for review again |
|
This PR actually doesn't reliably solve the problem of vs values not being written to the HF.bin file on RCH and we actually don't need it to work on RCH because we can just run it on NeSI where it always works properly, so I'll close this PR |
2 participants
This adds to @sungeunbae 's recent HF fixes
We were also finding that Vs30 values were sometimes missing from the output file, causing the BB stage to crash with a math error. The root cause seems to be that vs30 values were not completely written to the file before the program finished. To fix this, I've added flush and fsync, which should ensure that the writes have completed before the program finishes. I also added some validation checks for missing vs30 values at the end.