Raising errors when restart files don't exist

[sudarsan-surendralal]

Related to the concerns raised by #418. Implemented a base method to check this.

[freyso]

I like the approach to pull this into a generic function with proper testing opportunities.

[sudarsan-surendralal]

I've merged @freyso's warnings for SPHInX from #421. The tests will pass once pyiron/pyiron_base#511 is merged.

[samwaseda]

I see that it always the pattern:

self.check_if_file_exists(file_name)
new_ham.restart_file_list.append(posixpath.join(self.working_directory, file_name))

Doesn't it make sense to combine the two and do something like this?

def ensure_file_exists_and_return(self, filename) -> str:
    file_to_return = posixpath.join(self.working_directory, filename)
    if not os.path.isfile(file_to_return):
        raise FileNotFoundError(f"File {filename} not found in working directory: {self.working_directory}")
    return file_to_return

And

new_ham.restart_file_list.append(self.ensure_file_exists_and_return(file_name))

[sudarsan-surendralal]

I see that it always the pattern:

self.check_if_file_exists(file_name)
new_ham.restart_file_list.append(posixpath.join(self.working_directory, file_name))

Doesn't it make sense to combine the two and do something like this?

def ensure_file_exists_and_return(self, filename) -> str:
    file_to_return = posixpath.join(self.working_directory, filename)
    if not os.path.isfile(file_to_return):
        raise FileNotFoundError(f"File {filename} not found in working directory: {self.working_directory}")
    return file_to_return

And

new_ham.restart_file_list.append(self.ensure_file_exists_and_return(file_name))

Good idea! I've implemented this.

[freyso]

I agree about the functionality, but the function name is clumsy.
Since it basically identifies a file from the working directory, what about "find_workdir_file" or "get_workdir_file" ?

[freyso]

Btw - since you have already included the workdir fix from #421, do you allow me to transfer the 2nd half of #421 (re-raise error when main loop has failed) into this branch, and drop the 421 pull request? Trying to apply them both will produce merge conflicts.

[samwaseda]

Since it basically identifies a file from the working directory, what about "find_workdir_file" or "get_workdir_file" ?

Fine with get_workdir_file, although when the function is later generalized later I’m relatively sure that the function will get one more argument allowing the user to take a different path than the working directory.

[sudarsan-surendralal]

Btw - since you have already included the workdir fix from #421, do you allow me to transfer the 2nd half of #421 (re-raise error when main loop has failed) into this branch, and drop the 421 pull request? Trying to apply them both will produce merge conflicts.

Yes, sure.

[samwaseda]

Looks good to me :)

[freyso]

Btw - since you have already included the workdir fix from #421, do you allow me to transfer the 2nd half of #421 (re-raise error when main loop has failed) into this branch, and drop the 421 pull request? Trying to apply them both will produce merge conflicts.

Yes, sure.
oops, just realized that you already did this merge 2 hours ago...

[freyso]

I did the rename to get_workdir_file. Now I am happy.

[coveralls]

Pull Request Test Coverage Report for Build 1884536563

• 11 of 16 (68.75%) changed or added relevant lines in 4 files are covered.
• 3 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.03%) to 70.248%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pyiron_atomistics/lammps/base.py	0	2	0.0%
pyiron_atomistics/sphinx/base.py	0	3	0.0%

Files with Coverage Reduction	New Missed Lines	%
pyiron_atomistics/lammps/base.py	1	79.81%
pyiron_atomistics/vasp/base.py	2	65.45%

Totals
Change from base Build 1884182768:	0.03%
Covered Lines:	11685
Relevant Lines:	16634

---

💛 - Coveralls

[sudarsan-surendralal]

Well, with the new changes, all the unittests pass but the the sphinx notebook tests (this one) fails when attempting to restart since restart.out does not exist (or is compressed into a zip file after the job is finished). What's the correct way to handle this? @samwaseda @freyso

---------------------------------------------------------------------------
Exception encountered at "In [16]":
---------------------------------------------------------------------------
FileNotFoundError                         Traceback (most recent call last)
/tmp/ipykernel_5163/1135309357.py in <module>
----> 1 job = job.restart()
      2 job.run()

~/work/pyiron_atomistics/pyiron_atomistics/pyiron_atomistics/sphinx/base.py in restart(self, job_name, job_type, from_charge_density, from_wave_functions)
    820 
    821         if from_charge_density:
--> 822             new_job.restart_file_list.append(self.get_workdir_file("restart.out"))
    823 
    824         if from_wave_functions:

~/work/pyiron_atomistics/pyiron_atomistics/pyiron_atomistics/atomistics/job/atomistic.py in get_workdir_file(self, filename)
    436         appended_path = posixpath.join(self.working_directory, filename)
    437         if not os.path.isfile(appended_path):
--> 438             raise FileNotFoundError(f"File {filename} not found in working directory: {self.working_directory}")
    439         return appended_path
    440 

FileNotFoundError: File restart.out not found in working directory: /home/runner/work/pyiron_atomistics/pyiron_atomistics/SPX_CHECK_ALL/spx_Al_hdf5/spx_Al

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[samwaseda]

What's the current state of this PR? Shouldn't this be merged?

[sudarsan-surendralal]

What's the current state of this PR? Shouldn't this be merged?

The sphinx bandstructure notebooks/bandstructure.ipynb fails with the new changes since the charge density can't be found. Can you find out what's going on?

[samwaseda]

Oh ok I’ll do

[jan-janssen]

Looks good to me

[jan-janssen]

@sudarsan-surendralal I guess this is ready to be merged.