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

CI for openPMD #449

Merged
merged 13 commits into from
Jun 6, 2023
Merged

Conversation

franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented May 23, 2023

This adds openPMD-api to the CI infrastructure and adds a test for openPMD. At the moment, that test is test/shuffling_openpmd_test.py which tests openPMD-based shuffling.

It would be wise to add also a test that covers the basic functionality of openPMD I/O, especially also in parallel. Is there a test that uses numpy so far which I can modify to that purpose?

Update openPMD version in Dockerfile

Add openPMD output to clean.sh
Copy link
Contributor Author

@franzpoeschel franzpoeschel 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, I have some inline comments.
Given that parallel shuffling is now merged, should we maybe test that, too?

test/shuffling_test.py Outdated Show resolved Hide resolved
test/shuffling_test.py Outdated Show resolved Hide resolved
test/shuffling_openpmd_test.py Outdated Show resolved Hide resolved
@RandomDefaultUser RandomDefaultUser merged commit 83f03be into mala-project:develop Jun 6, 2023
@DanielKotik
Copy link
Member

@franzpoeschel @RandomDefaultUser If openPMD is an optional dependency for MALA, it should not be part of requirements.txt (instead add it here) and should only be added explicitly in the Dockerfile. If it is not an optional dependency, it should be present in requirements.txt and both mala_cpu_environment.yml and mala_cpu_base_environment.yml must then be updated accordingly. It must also be removed from the Dockerfile.

@RandomDefaultUser
Copy link
Member

I think we are moving for it not to be an optional dependency, so I would suggest we do the latter. I can open an issue for this.

@franzpoeschel
Copy link
Contributor Author

fixed by #541

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants