Replace RemoteStashFolderData with RemoteStashCopyData

[khsrali]

Fix: #6784
Deprecates the legacy stash node type RemoteStashFolderData and replace it with RemoteStashCopyData.
This is only for naming convention which we have discussed before with @agoscinski.

This PR also contains docstring cleanup for RemoteStashCompressedData

This PR also introduces a new property source_uuid. This is useful for un-stashing, and re-stashing purposes in the coming changes.

Note the 1 & 2 are entirely different changes, and deprecation is NOT occurred for introducing source_uuid.

[Copilot]

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.06%. Comparing base (48991ab) to head (07b37d6).
⚠️ Report is 29 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6825      +/-   ##
==========================================
+ Coverage   79.06%   79.06%   +0.01%     
==========================================
  Files         565      566       +1     
  Lines       43126    43155      +29     
==========================================
+ Hits        34095    34118      +23     
- Misses       9031     9037       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[khsrali]

Just talked to @agoscinski ,

We don't actually need to store the uuid actually. We could just use the traversals to reach there:

from aiida.orm import load_node, CalcJobNode, Node


def traverse(node: Node) -> CalcJobNode | None:
    for l in node.base.links.get_incoming():
        if isinstance(l.node, CalcJobNode):
            calcjob_node = l.node
            return calcjob_node
        return traverse(l.node)
    return None

node = load_node(12)
calcjob_node = traverse(node)
if None:
    raise ValueError("Your stash node is not connected to any calcjob node")
print(calcjob_node)

Therefore, this PR, has to be only do the deprecation job.

[khsrali]

@agoscinski I applied your previous review applied,
Please have a look on Monday, now it should be a quick review

[agoscinski]

Minor suggestions.

RemoteStashData should be actually an abstract class right? I mean it should never been used but only the child classes. Maybe we can do this in a subsequent PR? EDIT: just saw that we expose this publicly so I rather don't touch this class for now

[agoscinski]

Note that this does not conflict with querying old results using RemoteStashFolderData since it uses __new__ and not __init__

[khsrali]

The old data are from a different class, so it should not conflict with querying.
also this is a __init__ here. I don't understand the comment.

[agoscinski]

Anyone how has RemoteStashFolderData in the past AiiDA database for whatever reason, will construct this object during a query. If we raise an error in the __init__ and we would use __init__ to construct the object, then an error appears. However, the query builder does not invoke the __init__, it invokes only __new__ when creating the object

[agoscinski]

Could we maybe a test that checks if RemoteStashFolderData is still queryable? I checked that it works but the logic of the query builder is not trivial

from aiida.orm.nodes.data.remote.stash.folder import RemoteStashFolderData
from aiida.common import StashMode
RemoteStashFolderData(stash_mode=StashMode.COPY, target_basepath="/tmp", source_list=("/some",)).store()
QueryBuilder().append(RemoteStashFolderData).first()

[khsrali]

not possible to instantiate RemoteStashFolderData anymore, so your test here would fail.

It's a good thing that you are double checking for query builder, however since I have not removed MetadataFields I don't see why it should not be working

[agoscinski]

It still works, and it would be not good if it does not work. We need to support old databases with new versions. Please read this #6825 (comment) or try the code with your PR and see that it works. But nevertheless I would like a test that tests this for future changes.

[khsrali]

I'm confused, this is instantiating the deprecated class, it should raise 🤔

RemoteStashFolderData(stash_mode=StashMode.COPY, target_basepath="/tmp", source_list=("/some",)).store()

[agoscinski]

So the query builder uses

entity = RemoteStashFolderData.__new__(RemoteStashFolderData)
entity._backend_entity = orm.Int(5)._backend_entity
call_with_super_check(entity.initialize)

one can use this

from aiida.orm.nodes.data.remote.stash.folder import RemoteStashFolderData
entity = RemoteStashFolderData.__new__(RemoteStashFolderData)
entity._backend_entity = Int(5)._backend_entity
call_with_super_check(entity.initialize)
entity.store()
len(QueryBuilder().append(RemoteStashFolderData).all())

but this does not store them in the database, even though the store function is successful, I don't know why. I don't know the database backend enough. Any way we tested it. I think we should use database migration for this to replace it with RemoteStashCopyData

[agoscinski]

Sorry I thought the migrator also can change node types in the database, but it seems to just update the schema changes. I will think about a way to still store a RemoteStashFolderData

[khsrali]

I applied your review @agoscinski
We can talk in person about QB

[agoscinski]

Please read my comment #6825 (comment)

[agoscinski]

Also please just reply on

RemoteStashData should be actually an abstract class right? I mean it should never been used but only the child classes. Maybe we can do this in a subsequent PR?

[agoscinski]

Draft for commit, please check

`RemoteStashFolderData` has been so far only supported the `StashMode.COPY`
option and unlike the name suggests supported also to copy individual file(s).
To be more consistent with the naming scheme of `RemoteData` that also supports
files and folders and `RemoteStashCompressedData` we switch to the name
`RemoteStashCopyData`. For backwards compatibility reasons we keep
`RemoteStashFolderData` but disable the possibility of its construction and
introduce the new class `RemoteStashCopyData` that works exactly as
`RemoteStashFolderData`. This PR also contains docstring cleanup for
`RemoteStashCompressedData`.

[agoscinski]

We discussed that we need a migration for this as we do not want to have two datatypes in the database that basically do the same thing. Introducing the database migration in the future will causes problem as we need to resolve conflicts in the pk. No one in the team knows how the migration works and there is no guide that makes this straightforward. One can figure this out with the existing files and material but it will take a bit of time and this renaming is not essential so we postpone this PR from v2.7.0.

[khsrali]

We discussed that we need a migration for this as we do not want to have two datatypes in the database that basically do the same thing. Introducing the database migration in the future will causes problem as we need to resolve conflicts in the pk. No one in the team knows how the migration works and there is no guide that makes this straightforward. One can figure this out with the existing files and material but it will take a bit of time and this renaming is not essential so we postpone this PR from v2.7.0.

yes, pretty much that's all to be said 🤷‍♂️

[khsrali]

I close this for now.
The implementation of this commit might get used in a more broad PR that deprecates all RemoteStash*Data by removing abstractification in RemoteStashData