DataAccess implementation for shared volume architecture#376
DataAccess implementation for shared volume architecture#376
Conversation
| """ | ||
| print("--> Creating temporary folder on server for simulation inputs") | ||
| command = "mkdir %s" % self.TMP_DIR | ||
| command = "mkdir -p %s" % self.TMP_DIR |
There was a problem hiding this comment.
Just curious, why do you need the -p option here
There was a problem hiding this comment.
Couple reasons - create missing parent directories, and makes it re-runnable. Initially I got an error during prepare_simulation_input due to the tmp folder not existing yet. So this fixes that, and has the nice side effect of not failing when called twice.
There was a problem hiding this comment.
Yep, I had to deal with errors due to a missing tmp folder in my ssh server setup as well
| stderr=PIPE, | ||
| text=True, | ||
| ) | ||
| return wrap(None), wrap(proc.stdout), wrap(proc.stderr) |
There was a problem hiding this comment.
Not sure I understand the need for the first element of the returned tuple. Do you redirect the standard streams to dev/null?
There was a problem hiding this comment.
The implementation in SSHDataAccess uses exec_command from paramiko which returns the standard stream tuple with 3 elements - we have to match that here. We don't ever use the stdin component so I omitted that. The wrap is mostly for the stderr part which, when successful, will be None and cause an error when we call .readlines() - compared to paramiko which gives an empty stream. To fix that we just read from /dev/null here.
| :param str file_name: file name to copy. | ||
| :param str from_dir: data store directory to copy file from. | ||
| """ | ||
| pass |
There was a problem hiding this comment.
Do we need to overwrite the method here. It raises a NotImplementedError in the parent class, is that problematic?
There was a problem hiding this comment.
This does in fact override the behavior - in this case to do nothing, since the directories are symlinked already.
| dest = posixpath.join(self.root, to_dir, file_name) | ||
| self.check_file_exists(dest, should_exist=False) | ||
| self.copy(src, dest) | ||
| self.remove(src) |
There was a problem hiding this comment.
Should it be called move_to. I guess this would apply to DartaAccess and SSDataAccess
There was a problem hiding this comment.
Yeah, that makes more sense.
There was a problem hiding this comment.
You can keep copy_to without the remove if we need it in the future and define move_to that will call copy_to + the remove method
There was a problem hiding this comment.
We also call remove in the ssh implementation, so we only need move_to, right? In which case the change would just be renaming all the usages of copy_to.
There was a problem hiding this comment.
Yeah, right now we only move files.
jenhagg
force-pushed
the
jon/local_data
branch
from
edcc977 to
cbbd962
Compare
jenhagg
force-pushed
the
jon/local_data
branch
from
1f88217 to
37a1915
Compare
3 participants
Purpose
Add remaining pieces to enable expected functionality within the containerized setup (or any setup where reise.jl and powersimdata have access to a shared data volume).
What the code is doing
LocalDataAccesssubclass - logic is simplified via the symlink in the dockerfilecontext.pyto create the correct instanceTesting
Built the containers and run with docker-compose. Copy-pasted the instructions from the readme for launching a simulation.
Everything succeeds, up til the point where we invoke gurobi - so there is more testing to be done, but I think it's at a reasonable point, and shouldn't expect major changes here.Update: I'm able to launch a simulation now
Time estimate
20 min