-
Notifications
You must be signed in to change notification settings - Fork 40
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
Additional support for windows #475
Conversation
print(f"--> Moving file {src} to {dest}") | ||
self._check_file_exists(dest, should_exist=False) | ||
self.copy(src, dest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoiding the use of copy
because dest
is a file, and we don't it to automatically create /path/to/file.txt/
as a folder
powersimdata/scenario/delete.py
Outdated
@@ -41,25 +40,21 @@ def delete_scenario(self, confirm=True): | |||
self._scenario_list_manager.delete_entry(scenario_id) | |||
self._execute_list_manager.delete_entry(scenario_id) | |||
|
|||
wildcard = f"{scenario_id}_*" | |||
|
|||
print("--> Deleting scenario input data on server") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we delete locally when using plug. Printing "Deleting scenario input data" will then be always correct. Or perhaps, we can take advantage of the description
attribute to custom the message
powersimdata/scenario/delete.py
Outdated
print("--> Deleting scenario input data on server") | ||
target = posixpath.join(self.path_config.input_dir(), wildcard) | ||
target = self._data_access.match_scenario_files(scenario_id, "input") | ||
self._data_access.remove(target, recursive=False, confirm=confirm) | ||
|
||
print("--> Deleting scenario output data on server") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here
powersimdata/scenario/delete.py
Outdated
self._data_access.remove(tmp_dir, recursive=True, confirm=confirm) | ||
|
||
print("--> Deleting input and output data on local machine") | ||
target = os.path.join(server_setup.LOCAL_DIR, "data", "**", wildcard) | ||
target = os.path.join(server_setup.LOCAL_DIR, "data", "**", f"{scenario_id}_*") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do a LocalDataAccess().join
here and get rid of the import os
. It will be nice to be fully OS independent in the class.
powersimdata/scenario/execute.py
Outdated
) | ||
self.TMP_DIR = self._data_access.match_scenario_files(self.scenario_id, "tmp") | ||
|
||
def create_folder(self): | ||
"""Creates folder on server that will enclose simulation inputs.""" | ||
print("--> Creating temporary folder on server for simulation inputs") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the the temporary folder can be crated either on the local machine or the server. Perhaps we can the description
attribute here too.
raise IOError(f"Failed to copy {kind}.csv on server") | ||
from_dir = self._data_access.match_scenario_files(profile_as, "tmp") | ||
to_dir = self.TMP_DIR | ||
self._data_access.copy(f"{from_dir}/{kind}.csv", to_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is kind of confusing to move to
using relative path but to copy
with absolute path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I just didn't know a cleaner way to do it. The reason for this is mainly how we use copy
within move.py
, where the destination (backup disk) is not relative to the self.root
. Maybe we can revisit this in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
a4f286b
to
de99742
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Rebased and fixed the test that was still calling |
Purpose
Address several places that were not platform independent.
What the code is doing
execute_command
in the local case, as it relied on/bin/bash
DataAccess.join
attribute according to the subclass, removing hard codedposixpath.join
in calling codeget_base_dir
andmatch_scenario_files
to encapsulate the logic of creating paths, along with tests for theseInputData
andOutputData
which were relying on the values fromserver_setup.py
, which had posixpath implicitly hard coded (e.g.INPUT_DIR = data/input
). We delay the path join by using a tuple insteadTesting
LocalDataAccess
)SSHDataAccess
)Time estimate
20 mins