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

Split DataContainer to a HDF and non HDF part #1299

Merged
merged 7 commits into from
Feb 29, 2024
Merged

Split DataContainer to a HDF and non HDF part #1299

merged 7 commits into from
Feb 29, 2024

Conversation

pmrv
Copy link
Contributor

@pmrv pmrv commented Jan 30, 2024

Basically DataContainer is renamed to DataContainerBase and the methods used for HasHDF/HDFStub are moved to a new sub class DataContainer. Docstrings are replicated where necessary. No API changes for users.

In preparation of https://github.com/orgs/pyiron/projects/7/views/1

@liamhuber Pulling DataContainer out should be just a matter of copy/paste now.

Basically DataContainer is renamed to DataContainerBase and the methods
used for HasHDF/HDFStub are moved to a new sub class DataContainer.
Docstrings are replicated where necessary.
@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 Jan 30, 2024
@pmrv pmrv requested a review from liamhuber January 30, 2024 16:02
@liamhuber
Copy link
Member

@liamhuber Pulling DataContainer out should be just a matter of copy/paste now.

I think this may be of even more imminent utility to @samwaseda?

Will review shortly, but I peeked at the diff and it was what I intuitively expected so that should go pretty smoothly.

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.

99% as expected and no nits or anything.

The only thing I'm not sure of is the consistent use of type(self) for a bunch of recursion checks. Most of these seem to me like they should instead hard-code the class they're written in, so that if derived classes hold an instance of their parent class the recursion can continue uninterrupted. I'm not totally sure that's the right behaviour choice though.

At any rate, now that we explicitly have a base and child class, I think it's important to extend the tests to cover the case of child-holds-parent-instance to encode whatever behaviour decision you make.

pyiron_base/storage/datacontainer.py Show resolved Hide resolved
pyiron_base/storage/datacontainer.py Outdated Show resolved Hide resolved
pyiron_base/storage/datacontainer.py Outdated Show resolved Hide resolved
pyiron_base/storage/datacontainer.py Outdated Show resolved Hide resolved
Copy link
Member

@niklassiemer niklassiemer left a comment

Choose a reason for hiding this comment

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

LGTM once @liamhuber s concerns are matched

If we put a sub class of DataContainer(Base) into a base class, we still want to recurse all of it, so
checking against type(self) is not sufficient, since that would abort early.
@pmrv
Copy link
Contributor Author

pmrv commented Feb 1, 2024

 Traceback (most recent call last):
  File "/home/runner/work/pyiron_base/pyiron_base/tests/flex/test_pythonfunctioncontainer.py", line 63, in test_with_executor_wait
    self.assertTrue(job.server.future.done())
AssertionError: False is not true

@jan-janssen Sounds related to your recent developments. Would that be fixed by #1300 or #1301?

@jan-janssen
Copy link
Member

@jan-janssen Sounds related to your recent developments. Would that be fixed by #1300 or #1301?

The issue was solved by restarting the CI. We have to take a look at this and maybe increase the time out in case the issue continues to come up.

@jan-janssen jan-janssen added format_black reformat the code using the black standard and removed format_black reformat the code using the black standard labels Feb 1, 2024
@jan-janssen
Copy link
Member

Once this is merged, we should also release a new pyiron_base version.

@pmrv
Copy link
Contributor Author

pmrv commented Feb 1, 2024

@jan-janssen Sounds related to your recent developments. Would that be fixed by #1300 or #1301?

The issue was solved by restarting the CI. We have to take a look at this and maybe increase the time out in case the issue continues to come up.

I think it makes more sense to just use the timeout directly in the tests, then this can never happen.

@jan-janssen
Copy link
Member

@pmrv As more tests were requested, I changed the status of this pull request to draft until the tests are added.

@jan-janssen jan-janssen marked this pull request as draft February 14, 2024 12:46
pmrv added 3 commits February 28, 2024 11:08
Setting one value is not used in the older tests and would make the newer ones more complicated
@pmrv pmrv marked this pull request as ready for review February 28, 2024 13:27
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, type checking and testing changes lgtm.

@pmrv pmrv merged commit 4663e6d into main Feb 29, 2024
26 of 27 checks passed
@pmrv pmrv deleted the dclite branch February 29, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants