Skip to content
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

FIX: Discard lazy fields in template type checks #657

Closed
wants to merge 1 commit into from

Conversation

ghisvail
Copy link
Collaborator

@ghisvail ghisvail commented May 22, 2023

See issue #641 for context.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Summary

This PR discards lazy fields in checks performed in template updates.

I am not very happy with this change, but it's the only way I found so far to bypass this issue without major surgery of the code. Not being able to use output_file_template in a workflow is quite a bummer.

Open to other suggestions if any 🙏

Triage

from os import PathLike
from attrs import define, field
from pydra.engine import Workflow
from pydra.engine.specs import ShellSpec, SpecInfo
from pydra.engine.task import ShellCommandTask


class BackupFile(ShellCommandTask):
    """Example task performing a file backup with cp.

    If not target file is provided, it uses a default file suffixed by copy.
    """

    @define(kw_only=True)
    class InputSpec(ShellSpec):
        source_file: PathLike = field(
            metadata={"help_string": "source file", "mandatory": True, "argstr": ""}
        )

        target_file: str = field(
            metadata={"help_string": "target file", "argstr": "", "output_file_template": "{source_file}_copy"}
        )

    input_spec = SpecInfo(name="Input", bases=(InputSpec,))

    executable = "cp"


def backup(name="backup", **kwargs) -> Workflow:
    wf = Workflow(input_spec=["source_file", "target_file"], name=name, **kwargs)

    wf.add(
        BackupFile(
            name="backup_file",
            source_file=wf.lzin.source_file,
            target_file=wf.lzin.target_file,
        )
    )

    wf.set_output({"target_file": wf.backup_file.lzout.target_file})

    return wf


if __name__ == "__main__":
    from pathlib import Path
    from tempfile import TemporaryDirectory

    # Execute with standalone backup task.
    with TemporaryDirectory() as td:
        source_file = Path(td) / "source1.file"
        source_file.touch()

        task = BackupFile(source_file=source_file)
        result = task()

        print(result.output.target_file)

    # Execute backup task wrapped in a workflow.
    with TemporaryDirectory() as td:
        source_file = Path(td) / "source2.file"
        source_file.touch()

        task = backup(source_file=source_file)
        result = task()

        print(result.output.target_file)

Closes: #641

@ghisvail
Copy link
Collaborator Author

Without the fix, an exception is raised (Exception: target_file has to be str or bool, but LF('backup', 'target_file') set). With the fix, the path to the target file is printed.

@codecov
Copy link

codecov bot commented May 22, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.05 ⚠️

Comparison is base (426564e) 81.77% compared to head (4591be0) 81.72%.

❗ Current head 4591be0 differs from pull request most recent head f48eebb. Consider uploading reports for the commit f48eebb to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #657      +/-   ##
==========================================
- Coverage   81.77%   81.72%   -0.05%     
==========================================
  Files          20       20              
  Lines        4400     4400              
  Branches     1264        0    -1264     
==========================================
- Hits         3598     3596       -2     
- Misses        798      804       +6     
+ Partials        4        0       -4     
Flag Coverage Δ
unittests 81.72% <100.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pydra/engine/helpers_file.py 86.10% <100.00%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@djarecka
Copy link
Collaborator

if this works for you we can have it temporarily, but I would not close the issue. Sorry, was planning to look at this earlier, but will try to check it this week.

tclose added a commit to tclose/pydra that referenced this pull request Jun 29, 2023
@ghisvail
Copy link
Collaborator Author

Fixed by #662

@ghisvail ghisvail closed this Aug 10, 2023
@ghisvail ghisvail deleted the fix/issue-641 branch August 10, 2023 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with composing tasks with output_file_template in a workflow
2 participants