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

Remove automatic recursive to_dict/from_dict again #1632

Merged
merged 8 commits into from
Sep 4, 2024
Merged

Remove automatic recursive to_dict/from_dict again #1632

merged 8 commits into from
Sep 4, 2024

Conversation

pmrv
Copy link
Contributor

@pmrv pmrv commented Aug 30, 2024

This turned out to have negative interactions when writing and reading objects from GenericJob which currently employs a mixture of to_hdf and newer to_dict interfaces. When reading the objects again the recursive strategy can get confused which interface to use. Since it is anyway mostly relevant for the DataContainer, I have extracted this functionality into stand- alone functions that operate on the obj_dicts. Classes that wish to use it and do not confuse the two interfaces can call these functions in their implementations of _to_dict and _from_dict as done by DataContainer now.

pmrv and others added 2 commits August 30, 2024 17:14
This turned out to have negative interactions when writing and reading objects from GenericJob
which currently employs a mixture of to_hdf and newer to_dict interfaces.  When reading the
objects again the recursive strategy can get confused which interface to use.  Since it is
anyway mostly relevant for the DataContainer, I have extracted this functionality into stand-
alone functions that operate on the obj_dicts.  Classes that wish to use it and do not confuse
the two interfaces can call these functions in their implementations of _to_dict and _from_dict
as done by DataContainer now.
@pmrv pmrv added the integration Start the integration tests with pyiron_atomistics/contrib for this PR label Aug 30, 2024
@pmrv
Copy link
Contributor Author

pmrv commented Aug 30, 2024

@jan-janssen I will still update docs, but at least integration tests seem to pass now!
pmrv/debug-pyiron-stack#1

@pmrv pmrv marked this pull request as ready for review September 3, 2024 13:05
@pmrv
Copy link
Contributor Author

pmrv commented Sep 3, 2024

@jan-janssen What's your plan with #1613, should we merge that PR first or this one?

@jan-janssen
Copy link
Member

@pmrv can you rebase the integration tests to use the changes in this branch so we can validate the pyiron_atomistics tests pass? Then the order how we merge the pull requests does not really matter to me.

jan-janssen and others added 4 commits September 4, 2024 18:07
Previously we tried to install the deps of atomistics/contrib on top of
the base environment, now we install those dependencies first and then
add pyiron_base HEAD without dependencies.
The former breaks when there are incompatible versions, the latter can
break if base actually needs features or whole modules from its own
environment, but we're betting that that's rarer.
Update integration tests and a new one for notebooks
Copy link
Member

@jan-janssen jan-janssen left a comment

Choose a reason for hiding this comment

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

Looks good to me

@pmrv pmrv merged commit aec0e08 into main Sep 4, 2024
28 checks passed
@pmrv pmrv deleted the server branch September 4, 2024 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

2 participants