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 unnecessary database queries #1324

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

XzzX
Copy link
Contributor

@XzzX XzzX commented Feb 12, 2024

Improves performance of remove_jobs.

Comment on lines 1708 to 1709
if job_id not in self.get_job_ids(recursive=recursive):
continue
Copy link
Member

Choose a reason for hiding this comment

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

If I understand this pull request correctly, then the primary change is removing this database query, correct? Maybe we should split this pull request in two parts. One containing just the removal of this part and the other the refactoring. Having both mixed makes it a bit hard to read.

Copy link
Member

Choose a reason for hiding this comment

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

To clarify these changes a bit more I created two separate pull requests #1326 and #1325 - these contain the changes to the database queries but not the refactoring done in this pull request.

state.logger.debug(
"hdf file does not exist. Removal from database will be attempted."
)
self.db.delete_item(job.id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jan-janssen You missed this line. It might not be called often but it is another redundant database query.

Copy link
Member

Choose a reason for hiding this comment

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

But this is dangerous. For example when you create a job object and remove the HDF5 file, then the loading of the job object with inspect() fails and returns an IOError still in the except case the job object is not available, so we cannot use job.id.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I realised that the inspect() function also works when the file is corrupted #1327

@jan-janssen jan-janssen added the format_black reformat the code using the black standard label Feb 14, 2024
@jan-janssen jan-janssen merged commit 27ab330 into pyiron:main Feb 14, 2024
23 of 25 checks passed
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.

2 participants