Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix usage of RemoteData in arguments #107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/aiida_shell/calculations/shell.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ def process_arguments_and_nodes(
self.write_folder_data(node, dirpath, filename)
argument_interpolated = argument.format(**{placeholder: filename or placeholder})
elif isinstance(node, RemoteData):
self.handle_remote_data(node)
argument_interpolated = argument.format(**{placeholder: node.get_remote_path()})
else:
argument_interpolated = argument.format(**{placeholder: str(node.value)})

Expand Down
5 changes: 3 additions & 2 deletions tests/test_launch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,12 +140,13 @@ def test_nodes_remote_data(tmp_path, aiida_localhost, use_symlinks):
(dirpath_sub_filled / 'file_b.txt').write_text('content b')

# Create a ``RemoteData`` node of the ``archive`` directory which should contain only the ``archive.zip`` file.
remote_zip = RemoteData(remote_path=str(dirpath_archive / 'archive'), computer=aiida_localhost)
remote_data = RemoteData(remote_path=str(dirpath_archive), computer=aiida_localhost)

results, node = launch_shell_job(
'unzip',
arguments=['archive.zip'],
nodes={'remote': remote_data},
arguments=['{remote_zip}'],
nodes={'remote_zip': remote_zip, 'remote': remote_data},
Comment on lines +148 to +149
Copy link
Owner

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

Copy link
Contributor Author

@agoscinski agoscinski Jan 17, 2025 •
edited
Loading

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.

Copy link
Owner

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.

outputs=['file_a.txt'],
metadata={
'options': {'use_symlinks': use_symlinks},
Expand Down
Loading