aiida-core icon indicating copy to clipboard operation
aiida-core copied to clipboard

Replace `RemoteStashFolderData` with `RemoteStashCopyData`

Open khsrali opened this issue 8 months ago • 8 comments

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.~

khsrali avatar Apr 16 '25 12:04 khsrali

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 79.06%. Comparing base (48991ab) to head (07b37d6). :warning: Report is 83 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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Apr 17 '25 08:04 codecov[bot]

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 avatar May 02 '25 10:05 khsrali

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

khsrali avatar May 23 '25 15:05 khsrali

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

khsrali avatar Jun 10 '25 08:06 khsrali

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 avatar Jun 10 '25 09:06 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 avatar Jun 12 '25 09:06 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.

agoscinski avatar Jun 12 '25 09:06 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.

yes, pretty much that's all to be said :man_shrugging:

khsrali avatar Jun 15 '25 10:06 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

khsrali avatar Aug 12 '25 05:08 khsrali