Copy all run and out dirs at once, not in for-loop

Aariq commented 2 years ago

Addresses #3019 with changes to start_model_runs()

Description

Moves creation of directories on the remote host outside of for loop to do it all at once
Moves calls to remote.copy.to outside of for loop to copy over all /out and /run directories at once.
Moves call to remote.copy.from outside of for loop to copy all outputs back to local at once.
Adds notes for future improvments
Add informative errors to remote.copy.to() and remote.copy.from()

There aren't any tests for start_model_runs() and I'm not sure how to write them, but I did install this PR and test it with my own setup.

Motivation and Context

Review Time Estimate

[ ] Immediately
[ ] Within one week
[x] When possible

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[ ] My change requires a change to the documentation.
[x] My name is in the list of CITATION.cff
[x] I have updated the CHANGELOG.md.
[x] I have updated the documentation accordingly.
[x] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.

Aariq commented 1 year ago

Ok, seems to be working consistently now. Running two remote commands in a row seemed to be causing problems, but a 1 second pause seems to fix it. 🤷‍♂️

Aariq commented 1 year ago

Now that I've been using this for a while I've noticed that it still fails to copy over all the run/ files sometimes despite rsync reporting "success". This definitely works better than it did previously, but I can also continue to try to figure this out.

Aariq commented 1 year ago

Marked as draft again because I'm getting close (hopefully) to really fixing this.

Aariq commented 1 year ago

Ok, the remaining bug has nothing to do with PEcAn or with R for that matter. I've been able to reproduce it with just rsync. It also seems to be specific to our server as I can't reproduce it from my laptop. Going to open this back up for review. One possible safeguard is to wrap the remote.copy.to commands in PEcAn.utils::retry.func() but I'll wait to get feedback before doing this.

mdietze commented 1 year ago

@robkooper could you look at the changes you requested so we can figure out whether this is ready to pull?

PecanProject / pecan