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

Fixes for parallel master without database #1156

Merged
merged 8 commits into from
Jul 8, 2023
Merged

Conversation

jan-janssen
Copy link
Member

@jan-janssen jan-janssen commented Jul 5, 2023

Example Code:

import pyiron_gpl
from pyiron_atomistics import Project

pr = Project("elastic")
pr.remove_jobs(silently=True, recursive=True)

potential = '2008--Hepburn-D-J--Fe-C--LAMMPS--ipr1'
structure = pr.create.structure.ase.bulk("Fe", cubic=True)

lmp_mini = pr.create_job(pr.job_type.Lammps, "lmp_mini")
lmp_mini.structure = structure
lmp_mini.potential = potential
lmp_mini.calc_minimize(pressure=0.0)
lmp_mini.run()

lmp_elastic = pr.create_job(pr.job_type.Lammps, "lmp_fe_elastic")
lmp_elastic.structure = lmp_mini.get_structure()
lmp_elastic.potential = potential
elastic_fe = lmp_elastic.create_job(pr.job_type.ElasticMatrixJob, "elastic_fe")
elastic_fe.run()

elastic_fe["output/elasticmatrix"]["C"]

My .pyiron config:

[DEFAULT]
RESOURCE_PATHS = /Users/janssen/pyiron/resources
PROJECT_CHECK_ENABLED = False
DISABLE_DATABASE = True

@jan-janssen jan-janssen added the format_black reformat the code using the black standard label Jul 5, 2023
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.

These changes look got to me, but I think it's important to encode something equivalent to your github example into the tests (at least the integration tests) so that this stays working. E.g. what happens when some well-meaning but less-informed dev comes along in the future and says "ah, self.project_hdf5.h5_path is basically just self.job_name so I can simplify this code a little bit".

Overall database-free running is woefully tested. #951 resulted in multiple other PRs to fix FileTable because no one had had the audacity to save and load a job without the database in the tests before then. Since you now found an edge case here where we previously had bad behaviour, let's take the opportunity to improve our tests.

@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 Jul 5, 2023
@jan-janssen
Copy link
Member Author

These changes look got to me, but I think it's important to encode something equivalent to your github example into the tests (at least the integration tests) so that this stays working. E.g. what happens when some well-meaning but less-informed dev comes along in the future and says "ah, self.project_hdf5.h5_path is basically just self.job_name so I can simplify this code a little bit".

I added comments to explain the changes

Overall database-free running is woefully tested. #951 resulted in multiple other PRs to fix FileTable because no one had had the audacity to save and load a job without the database in the tests before then. Since you now found an edge case here where we previously had bad behaviour, let's take the opportunity to improve our tests.

I agree but I see adding the tests for the FileTable part as a different topic, these are really just fixes to get pyiron running, no new features, so I want to make sure they are included in the next release. I am going to work on adding tests for the FileTable, but that is going to be another pull request and presumably might still take a bit more time.

@liamhuber
Copy link
Member

I agree but I see adding the tests for the FileTable part as a different topic, these are really just fixes to get pyiron running, no new features, so I want to make sure they are included in the next release. I am going to work on adding tests for the FileTable, but that is going to be another pull request and presumably might still take a bit more time.

Adding tests for the FileTable is one thing, but here we expect parallel master to behave a certain way. You discovered and currently have in your mental RAM how and why it is not behaving the way you expected. You presumably have a notebook that guarantees these changes enforce the behaviour you want (at a minimum you have the snippet at the head of this PR). I do not understand why these bug fixes should be in a separate PR from tests ensuring that these bugs stay fixed.

I've got other stuff so I'm going to stop checking on this PR. If you add tests, please ping me and I'll re-review/approve this PR. If you're unwilling to add tests corresponding to the behaviour in your snippet above, go ahead and use admin privileges to merge despite my review -- I'm not willing to have an argument about it and won't give you any further trouble on the issue, but I'm also not willing to approve the bug fixes sans tests.

@jan-janssen jan-janssen merged commit c6e0223 into main Jul 8, 2023
22 checks passed
@delete-merged-branch delete-merged-branch bot deleted the parallel_master_no_db branch July 8, 2023 12:46
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants