Fix symlinking `RemoteData` nodes for remote-submission #136

GeigerJ2 · 2025-03-27T15:23:50Z

The issue we were encountering (see here) is resolved by specifying the filenames argument of the ShellTask.

Notes:

Removed code for sourcing files to set env variables, as it is broken anyway for remote submission
small test case currently fails as restart output not retrieved -> Root of problem might lie elsewhere, though
Enforce absolute path for available data inputs?!

GeigerJ2 · 2025-04-10T08:19:45Z

Ping to the team, @leclairm, @agoscinski, @DropD, that this should be ready for review.

GeigerJ2 · 2025-04-10T09:16:47Z

src/sirocco/workgraph.py

+        filenames = {}
+        input_sockets = workgraph_task.inputs.nodes
+
+        for input_socket in input_sockets:


Check that this doesn't mess with SinglefileData.

src/sirocco/workgraph.py

GeigerJ2 · 2025-04-22T07:35:34Z

pyproject.toml

  "lxml",
-  "f90nml"
+  "f90nml",
+  "aiida-shell @ git+https://github.com/sphuber/aiida-shell.git@fix/105/handle-remote-data-argument-placeholders",


Need to pin here before the PR is merged and released.

Do we need this change in aiida-shell or would this just make our life easier?

Yeah, this is required. Without it, the instructions will always be appended by /*, so will always fail for our RemoteData files, see here:
https://github.com/sphuber/aiida-shell/blob/189df631759eb07e574bfb5a5be2843ea532ec9d/src/aiida_shell/calculations/shell.py#L341
We could still hack our way around it by using the parent of the src path, but then, still, all files in the directory will be linked. I think it's better if we keep it pinned for now, and expect the PR to aiida-shell to be merged and released.

Can't we monkeypatch ShellJob and pin the exact version? When we start to include git hashes in our dependencies I am afraid we run into problems later on. Dependency resolvement becomes a bit intransparent and unpredictable, e.g. we pin a hash while wrokgraph pins a version.

Yeah, that was more meant as a temporary fix. Maybe @sphuber can include the change in a patch release of aiida-shell, and we can set that?

I finally released this fix in v0.8.1 just now. Had forgotten about it, sorry for the delay

Cheers, thanks a lot @sphuber! Just in time that this PR actually works and can be reviewed 🚀

GeigerJ2 · 2025-04-22T07:35:50Z

pyproject.toml

 ## Hatch configurations

+[tool.hatch.metadata]
+allow-direct-references = true


Needed for being able to pin aiida-shell.

tests/test_wc_workflow.py

src/sirocco/parsing/yaml_data_models.py

src/sirocco/workgraph.py

agoscinski · 2025-05-05T20:05:40Z

src/sirocco/workgraph.py

+
+        filenames = {}
+
+        for input_list in task.inputs.values():


inputs can be a list of inputs because for shell task we can assign multiple inputs for one port and the list returns you all the inputs for this one port. I don't think we stress test this much so most of the time the input_list has just one element.

Suggested change

for input_list in task.inputs.values():

for input_list in task.input_data_nodes:

I think you need to handle all inputs separate as these can be potentially different files. Note that currently our code is also buggy in this regard. If we pass the outputs of a parametrized shell task to another task aiida will copy the file to the working directory. Since the output filename is the same for each parameter they will be overwritten. So only one file exists in the working directory of the next tasks that gets this as input. The solution is to give the outputs unique names.

tests/test_wc_workflow.py

tests/cases/small/config/config.yml

src/sirocco/workgraph.py

Also, fix hatch

agoscinski · 2025-05-11T06:32:45Z

Can you rebase so we can run the tests?

I decided to remove the check since we will need to move it to to the aiida part after PR #136 is merged. With #136 we need to consider remote data which we cannot check neigher in core nor parsing. So moving this now to the yaml parsing would require the adoption of many test which will be anyway not be needed.

GeigerJ2 · 2025-06-03T05:16:27Z

src/sirocco/workgraph.py

                msg = f"Could not find computer {data.computer!r} for input {data}."
                raise ValueError(msg) from err
-            self._aiida_data_nodes[label] = aiida.orm.RemoteData(remote_path=data.src, label=label, computer=computer)
+            # `remote_path` must be str not PosixPath to be JSON-serializable


This was a bug in the code before this PR that surfaced when actually submitting.

… make tests pass.

GeigerJ2 · 2025-06-03T08:26:46Z

pyproject.toml

 [tool.pytest.ini_options]
 # Configuration for [pytest](https://docs.pytest.org)
-addopts = "--pdbcls=IPython.terminal.debugger:TerminalPdb"
+addopts = "-s --pdbcls=IPython.terminal.debugger:TerminalPdb"


-s disables output capturing, and allows for setting breakpoints in test code

GeigerJ2 · 2025-06-03T10:47:42Z

Superseeded by PR #157 from branch on upstream.

- Add dispatching for `IconTask` in multiple functions in responsible for creating the WorkGraph. - Pass `computer` argument to `core.Task` since it is required for the creation of a `IconCaclculation`. In the `small-shell` config the usage of `computer` has been removed temporarly until PR #136 fixes the usage. - Changes in `core.AvailableData`: - Use `config_rootdir` to resolve location of data if relative. - The `src` member is now compulsory and validated. This change required to implement the `from_config` constructor for `AvailableData` and `GeneratedData` as the validation does not happen for `GeneratedData`. - Fixing how `is_restart` is determined: It was not considering the data structure correctly data items. It was only checking the existance of the restart key in the input items, it was, however, not considering that the data structure still lists the input item with an empty list when the `when` keyword is used. Now it validates correctly to `False`. - In tests we use now `pytest.fixture.usefixtures` when a fixture is not directly used but is required to be executed

I decided to remove the check since we will need to move it to to the aiida part after PR #136 is merged. With #136 we need to consider remote data which we cannot check neigher in core nor parsing. So moving this now to the yaml parsing would require the adoption of many test which will be anyway not be needed.

- Add dispatching for `IconTask` in multiple functions in responsible for creating the WorkGraph. - Pass `computer` argument to `core.Task` since it is required for the creation of a `IconCaclculation`. In the `small-shell` config the usage of `computer` has been removed temporarly until PR #136 fixes the usage. - Changes in `core.AvailableData`: - Use `config_rootdir` to resolve location of data if relative. - The `src` member is now compulsory and validated. This change required to implement the `from_config` constructor for `AvailableData` and `GeneratedData` as the validation does not happen for `GeneratedData`. - Fixing how `is_restart` is determined: It was not considering the data structure correctly data items. It was only checking the existance of the restart key in the input items, it was, however, not considering that the data structure still lists the input item with an empty list when the `when` keyword is used. Now it validates correctly to `False`. - In tests we use now `pytest.fixture.usefixtures` when a fixture is not directly used but is required to be executed

I decided to remove the check since we will need to move it to to the aiida part after PR #136 is merged. With #136 we need to consider remote data which we cannot check neither in core nor in parsing. So moving this now to the yaml parsing would require the adoption of many test which will be anyway not be needed.

Adds a new test case `small-icon` and renamed test case `small` to `small-shell`. The `small-icon` test case is a copy of `small` replacing the usage of the dummy icon scripts with real icon. Because the `small` test case increases coverage of `ShellTask` it is kept. - Add dispatching for `IconTask` in multiple functions in responsible for creating the WorkGraph. - Pass `computer` argument to `core.Task` since it is required for the creation of a `IconCaclculation`. In the `small-shell` config the usage of `computer` has been removed temporarly until PR #136 fixes the usage. - Changes in `core.AvailableData`: - Use `config_rootdir` to resolve location of data if relative. - The `src` member is now compulsory and validated. This change required to implement the `from_config` constructor for `AvailableData` and `GeneratedData` as the validation does not happen for `GeneratedData`. - Fixing how `is_restart` is determined: It was not considering the data structure correctly data items. It was only checking the existance of the restart key in the input items, it was, however, not considering that the data structure still lists the input item with an empty list when the `when` keyword is used. Now it validates correctly to `False`. - In tests we use now `pytest.fixture.usefixtures` when a fixture is not directly used but is required to be executed - Exclude `test/cases/*` in type check as it contains dummy python scripts - Update the default options of `hatch test` to run without icon - Rename pytest fixture `icon_grid_simple_path` to `icon_grid_path`

GeigerJ2 force-pushed the remote-submission branch from db22cf4 to e3be067 Compare March 27, 2025 15:27

This was referenced Mar 27, 2025

Add computer to Data initialization in core #92

Closed

Handling AvailableData entries that end up as RemoteData #132

Closed

GeigerJ2 changed the title ~~WIP: Remote submission~~ Fix symlinking RemoteData nodes for remote-submission Apr 10, 2025

GeigerJ2 commented Apr 10, 2025

View reviewed changes

leclairm reviewed Apr 10, 2025

View reviewed changes

src/sirocco/workgraph.py Outdated Show resolved Hide resolved

GeigerJ2 commented Apr 22, 2025

View reviewed changes

tests/test_wc_workflow.py Outdated Show resolved Hide resolved

GeigerJ2 mentioned this pull request Apr 25, 2025

Update dependency aiida-workgraph to v0.5.2 #137

Merged

GeigerJ2 requested a review from agoscinski April 25, 2025 09:11

agoscinski requested changes May 5, 2025

View reviewed changes

agoscinski reviewed May 5, 2025

View reviewed changes

src/sirocco/workgraph.py Outdated Show resolved Hide resolved

GeigerJ2 force-pushed the remote-submission branch 3 times, most recently from d16e396 to 965d606 Compare May 6, 2025 11:13

GeigerJ2 mentioned this pull request May 6, 2025

Make sure copied/symlinked filenames are different in target workdir #144

Closed

GeigerJ2 added 3 commits May 6, 2025 14:14

Actual implementation changes

847c96e

Add specific tests for filenames argument.

dabeed4

Add checks for computer existence for filenames

a2ca536

Also, fix hatch

GeigerJ2 force-pushed the remote-submission branch from c4b98d0 to a2ca536 Compare May 6, 2025 12:14

GeigerJ2 added 5 commits May 8, 2025 14:29

fix tests

ee3cbff

Replace None key with src

996ea7b

Add expected arguments list for comparison.

2092627

Verify with nodes

48318a2

Add minimal CLI interface using typer and rich

5ac98bf

GeigerJ2 added 2 commits May 12, 2025 11:14

Merge remote-tracking branch 'upstream/main' into remote-submission

23eeaaa

Merge remote-tracking branch 'upstream/main' into remote-submission

7c9ed55

GeigerJ2 and others added 12 commits June 2, 2025 14:34

Uncomment out previous implementation and duplicate test

5febd4f

Merge in CLI for easier development

00d2eb9

Implementation seems to work now

4be222f

.

84677c7

.

47e361d

.

7d84dd4

.

31268f0

.

221cfc9

.

a51ab9a

.

c149a10

.

80948e6

hatch fmt and types:check pass

c230667

GeigerJ2 commented Jun 3, 2025

View reviewed changes

GeigerJ2 and others added 6 commits June 3, 2025 08:57

.

b4ca8ff

.

4807db6

.

6a42fb8

Allow and properly resolve relative AvailableData src on localhost to…

3676b14

… make tests pass.

.

5237cdf

.

7c3bd68

GeigerJ2 commented Jun 3, 2025

View reviewed changes

GeigerJ2 added 2 commits June 3, 2025 10:29

.

a565fb1

Remove erroneously introduced output renaming.

83aea88

GeigerJ2 closed this Jun 4, 2025

	for input_list in task.inputs.values():
	for input_list in task.input_data_nodes:

Fix symlinking RemoteData nodes for remote-submission #136

Fix symlinking RemoteData nodes for remote-submission #136

Uh oh!

Conversation

GeigerJ2 commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GeigerJ2 commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agoscinski commented May 11, 2025

Uh oh!

GeigerJ2 Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeigerJ2 commented Jun 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix symlinking `RemoteData` nodes for remote-submission #136

Fix symlinking `RemoteData` nodes for remote-submission #136

GeigerJ2 commented Mar 27, 2025 •

edited

Loading

GeigerJ2 commented Apr 10, 2025 •

edited

Loading

GeigerJ2 Jun 3, 2025 •

edited

Loading