-
Couldn't load subscription status.
- Fork 7
Fix usage of RemoteData in arguments
#107
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
base: master
Are you sure you want to change the base?
Conversation
29062ad to
b8506f7
Compare
b8506f7 to
8a9e7d4
Compare
The tests did not cover the usage of `RemoteData` in the arguments which did not cover the bug that `self.handle_remote_data` does not exist. We translate the node by passing the path to the arguments.
8a9e7d4 to
0341582
Compare
|
ping @sphuber |
| arguments=['{remote_zip}'], | ||
| nodes={'remote_zip': remote_zip, 'remote': remote_data}, |
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 am not quite sure I understand these changes. I already create the archive on line 133. It takes the dir at dirpath_source and creates the file archive.zip inside the dirpath_archive folder. I then create a RemoteData of dirpath_archive, which therefore includes the archive.zip file, and add archive.zip directly to the unzip arguments. This is now a relative path, which should be there since the RemoteData remote_data should be copied there. If I understand things correctly, you are now creating another RemoteData and also adding that. I don't understand why
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.
Ah I see, I was not sure what the rest does. Is the symlink an effect of the RemoteData being present in the nodes? That it automatically links or copies the nodes into the internal aiida run directory? I did used Remote data similar to regular SinglefileData nodes which did not work
results, node = launch_shell_job(
'/Users/alexgo/code/Sirocco/tests/cases/small/config/scripts/icon.py',
arguments = ["--init", "{init}"],
nodes = {
"init": RemoteData(remote_path="/Users/alexgo/code/Sirocco/tests/cases/small/config/data/initial_conditions", computer=load_computer("localhost")) # does not work
#"init": SinglefileData.from_string("") # does work
},
metadata={'options': {'use_symlinks': True}},
)and therefore tried to fix it in this PR. However when I use it like it was in test before it still does not work
results, node = launch_shell_job(
'/Users/alexgo/code/Sirocco/tests/cases/small/config/scripts/icon.py',
arguments = ["--init", "initial_conditions"],
nodes = {
"remote": RemoteData(remote_path="/Users/alexgo/code/Sirocco/tests/cases/small/config/data/initial_conditions", computer=load_computer("localhost")) # should be now symlinked to the aiida running directory right?
},
metadata={'options': {'use_symlinks': True}},
)because I refer to a file in RemoteData and not a directory. So if I refer to the directory it works
results, node = launch_shell_job(
'/Users/alexgo/code/Sirocco/tests/cases/small/config/scripts/icon.py',
arguments = ["--init", "initial_conditions"],
nodes = {
"remote": RemoteData(remote_path="/Users/alexgo/code/Sirocco/tests/cases/small/config/data", computer=load_computer("localhost")) # should be now symlinked to the aiida running directory right?
},
metadata={'options': {'use_symlinks': True}},
)But for my use case I only want to refer to a file and not a folder. Does the current implementation offer a way that I can refer only to one file with RemoteData? Otherwise I would go on with this PR and instead of passing the full directory, I would also create a symlink or copy (depending on the use_symlinks option) of the file to be consistent in the aiida runnign directory, as well as creating a new test because this is testing for something else.
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.
Note that I also had already started working on a fix. There were a few problems with the RemoteData handling. It wasn't possible to specify a target subdirectory and if a remote data had a placeholder in the arguments it was not correctly handled. I have a WIP branch here: #108
Just need to fix a final bug in the generated remote copy list.
The tests did not cover the usage of
RemoteDatain the arguments which did not cover the bug thatself.handle_remote_datadoes not exist. We translate the node by passing the path to the arguments.Mentioned in #105 (comment)