-
Notifications
You must be signed in to change notification settings - 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
RemoteData
in the arguments which did not cover the bug thatself.handle_remote_data
does not exist. We translate the node by passing the path to the arguments.Mentioned in #105 (comment)