Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Getting syncdbAfterWrite working #1857

Merged
merged 7 commits into from
Sep 12, 2024

Conversation

zachmprince
Copy link
Contributor

@zachmprince zachmprince commented Sep 5, 2024

What is the change?

This PR modifies Database3.syncToSharedFolder so that it closes the h5py.File before trying to copy the h5 file to the working directory. The full process is:

  1. Call h5py.File.close() and set the object to None.
  2. Copy the h5py file to the working directory.
  3. Garbage collect to avoid multiple database instances in memory.
  4. Reload the h5 file in append mode

Tests were also added to ensure the syncDbAfterWrite setting behaves as expected.

Why is the change being made?

Partial resolution to #1794

Previously, setting syncDbAfterWrite caused a PermissionError due to the h5 file not being closed before trying to copy it.


Checklist

  • The release notes have been updated if necessary.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in pyproject.toml.

@zachmprince zachmprince added the bug Something is wrong: Highest Priority label Sep 5, 2024
zachmprince and others added 2 commits September 6, 2024 15:23
Co-authored-by: John Stilley <1831479+john-science@users.noreply.github.com>
@john-science john-science self-requested a review September 9, 2024 23:44
@john-science
Copy link
Member

@zachmprince Remind me, did we run those downstream tests successfully? Last I heard, you sounded convinced, but were worried about some unrelated errors? How are you feeling now?

@zachmprince
Copy link
Contributor Author

I am confident these changes are not affecting downstream applications. Test failure I ran into is unrelated. If you are okay merging it, please do! It's all okay on my side.

@john-science john-science merged commit 57dc6fe into terrapower:main Sep 12, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong: Highest Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants