-
Notifications
You must be signed in to change notification settings - Fork 14
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 GPU support #1126
Add GPU support #1126
Conversation
Waiting for conda-forge/status#144 |
@@ -490,6 +508,8 @@ def from_hdf(self, hdf, group_name=None): | |||
self._threads = hdf_dict["threads"] | |||
if "additional_arguments" in hdf_dict.keys(): | |||
self.additional_arguments = hdf_dict["additional_arguments"] | |||
if "gpus" in hdf_dict.keys(): | |||
self._gpus = hdf_dict["accept_crash"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why the names don't match and clash with an already existing entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the PRs merged over the last two days, I'm also quite concerned that we now have both Flux and GPU "support", but the tests and notebooks directories have not been touched at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - I fixed it in #1127
@liamhuber the challenging part for both is the integration in the test environment. But I agree I am going to work on adding tests once the presentation on Monday was successful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For flux it seems super straightforward: add flux
to the notebook dependencies, take your example from #1120 and change it so it uses a python template job instead of atomistics, and add it to an example notebook. That would give us both a bare-minimum of testing and at least somewhere in the codebase that the intended use is shown.
For GPUs I agree, we cannot directly test the execution on GitHub CI, but we could at least have AssertRaises
tests to make sure that setting the gpus
flag actually gets us to the right part of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I agree I am going to work on adding tests once the presentation on Monday was successful.
@jan-janssen I am super not comfortable with merges to base being done under "I have a presentation on Monday" pressure. Is it not possible to leave these in a branch and do the example from there, or is your example really getting the audience to conda install pyiron
such that you need these changes publicly available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is your example really getting the audience to
conda install pyiron
such that you need these changes publicly available?
Yes, that is the goal - tell people during the three month we developed an Exascale ready version of pyiron and they can now install it directly from conda, with flux and all required dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For flux it seems super straightforward: add
flux
to the notebook dependencies, take your example from #1120 and change it so it uses a python template job instead of atomistics, and add it to an example notebook. That would give us both a bare-minimum of testing and at least somewhere in the codebase that the intended use is shown.
Tests and documentation are in progress - I add them soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is the goal - tell people during the three month we developed an Exascale ready version of pyiron and they can now install it directly from conda, with flux and all required dependencies.
Well, for me this only strengthens the case that the tests should be present at the time the functionality is merged, but in any case I wish you a smooth presentation.
No description provided.