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

Add maintenance function to update TYPE in HDF5 files #1243

Merged
merged 13 commits into from
Dec 7, 2023
Merged

Add maintenance function to update TYPE in HDF5 files #1243

merged 13 commits into from
Dec 7, 2023

Conversation

pmrv
Copy link
Contributor

@pmrv pmrv commented Dec 5, 2023

Problem as discussed in #1234.

This adds pyiron_base.project.maintenance.add_module_conversion that takes to module paths and saves them for updates. Upon calling the corresponding maintenance function HDF5 files (jobs and project.data) are scanned and the TYPE field is updated, substituting the old for the new module path. When an object is loaded and the import fails while its module path is known to have been updated a readable error message is raised directing users to use the maintenance function.

In the future whenever classes are moved between packages and modules a corresponding call to add_module_conversion has to be inserted into the __init__ of the package where those classes were previously defined.

@pmrv pmrv added the enhancement New feature or request label Dec 5, 2023
@pmrv pmrv requested review from jan-janssen and liamhuber December 5, 2023 12:09
@pmrv pmrv added integration Start the integration tests with pyiron_atomistics/contrib for this PR format_black reformat the code using the black standard labels Dec 5, 2023
pyiron-runner and others added 2 commits December 5, 2023 12:22
PyironTable save analysis functions as pickled objects.  Afterwards we
moved them to different modules so unpickling breaks.  This method now
imports all the updated modules, reloads and rewrites the tables back,
so that on the next load they can be unpickled from the new locations.
@pmrv
Copy link
Contributor Author

pmrv commented Dec 5, 2023

Reminder to myself: forgot to add new test file.

Copy link
Member

@liamhuber liamhuber left a comment

Choose a reason for hiding this comment

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

Nice. I have some questions/suggestions, but they're "accept with minor revision not requiring re-review" 😜 I also assume you'll get the integration tests passing, but I see an open PR over on pyiron_atomistics on this topic, so I am confident you're already on top of this 👍

One thing on my wishlist is that we actually have an old project that we test the updates on? Maybe we have this and I'm just not aware? If such a thing is indeed missing, I can accept that it's outside scope for this PR.

def add_module_conversion(old: str, new: str):
"""
Add a new module conversion.
After this call any object that was previously defined in module `old` and is now defined in `new`, will be
Copy link
Member

Choose a reason for hiding this comment

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

This is not strictly true, or? It looks to me like you need to invoke add_module_conversion then still do a maintenance cycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, I had just copied this over from #1234, my bad.

pyiron_base/project/maintenance.py Show resolved Hide resolved
pyiron_base/storage/hdfio.py Show resolved Hide resolved
job.run()

# manipulate type to fake an old module
old_path_job = "job.module"
Copy link
Member

Choose a reason for hiding this comment

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

Beautiful. Even my stretch goal for this PR is satisfied 😂 🚀

@pmrv
Copy link
Contributor Author

pmrv commented Dec 7, 2023

From my side this is done and I'll merge it tonight and bump the version if there's no other comments.

@jan-janssen Is it realistic for you update the PR on contrib until Monday?

pyiron/pyiron_contrib#887

@pmrv
Copy link
Contributor Author

pmrv commented Dec 7, 2023

Compatibility tests are failing because of some mamba issues.

@pmrv pmrv merged commit 56a2fba into main Dec 7, 2023
23 of 25 checks passed
@delete-merged-branch delete-merged-branch bot deleted the remap_main branch December 7, 2023 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request format_black reformat the code using the black standard integration Start the integration tests with pyiron_atomistics/contrib for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants