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

Dictionaries and tuples with AppFutures do not get implicitly resolved #3108

Open
Andrew-S-Rosen opened this issue Feb 28, 2024 · 2 comments
Labels

Comments

@Andrew-S-Rosen
Copy link
Contributor

Andrew-S-Rosen commented Feb 28, 2024

Describe the bug

When passing a dict or tuple (or other iterable) with an AppFuture in it as the input to a PythonApp, Parsl does not implicitly know to block and resolve it before proceeding with the next task.

import parsl
from parsl import python_app
from pathlib import Path

parsl.load()

@python_app
def job1():
    return Path.cwd()

@python_app
def job2(t: tuple[Path, str]):
    Path(t[0], t[1]).touch()

job2((job1(), "hello")).result()
import parsl
from parsl import python_app
from pathlib import Path

parsl.load()

@python_app
def job1():
    return Path.cwd()

@python_app
def job2(d: dict[Path, str]):
    for k,v in d.items():
        Path(k,v).touch()

job2({job1(): "hello"}).result()

In both scenarios, you will get the same TypeError: expected str, bytes or os.PathLike object, not AppFuture because the AppFuture is not automatically resolved by the time it gets to the Path() call.

Note to self that we should also confirm the following works once the simpler case is resolved:

import parsl
from parsl import python_app
from pathlib import Path

parsl.load()

@python_app
def job1():
    return {"directory": Path.cwd()}

@python_app
def job2(t: tuple[Path, str]):
    Path(t[0], t[1]).touch()

job2((job1()["directory"], "hello")).result()
@Andrew-S-Rosen Andrew-S-Rosen changed the title The __getitem__ magic with AppFutures has a simple failure mode The __getitem__ lifted operator magic with AppFutures has a simple failure mode Feb 28, 2024
@Andrew-S-Rosen Andrew-S-Rosen changed the title The __getitem__ lifted operator magic with AppFutures has a simple failure mode Dictionaries and tuples with AppFutures do not get implicitly resolved Feb 28, 2024
@Andrew-S-Rosen
Copy link
Contributor Author

Andrew-S-Rosen commented Feb 29, 2024

@yadudoc --- I'd be interested to get your thoughts on this one. It seems you have had similar ideas in prior issues, such as in #1790 (comment). I would love to see at least some degree of Parsl-based introspection, and I'm curious what your perspective is.

@Andrew-S-Rosen
Copy link
Contributor Author

My proposal would be the following: if an object __contains__ an AppFuture, then Parsl should know to implicitly block and resolve. This would cover the keys of dictionaries, and first level of lists and tuples, among other iterables. It would not be recursive, but I'd argue that this is better than nothing at all.

Andrew-S-Rosen added a commit to Quantum-Accelerators/quacc that referenced this issue Feb 29, 2024
Closes #1742.

Note: 
- Parsl/parsl#3108

## Motivation

In short, the `copy_files` handling was in desperate need of a
behind-the-scenes overhaul. There are several reasons for this.

1. As noted in #1742, you can't do `Path()` on a `Future`, which means
any recipes doing `Path()` (or string concatenation of paths) in a
`Flow` are going to crash when dispatched over a workflow engine.
2. There are several file handling functions that do redundant things or
are handled in existing dependencies: `copy_decompress_files`,
`copy_decompress_tree`, `copy_decompress_files_from_dir`.
3. Because of Point 2, not all the functions handle things in the same
way. For instance, only some have glob support right now, which is
confusing.
4. There are many instances where the type hints are simply inaccurate.
For instance, in the Espresso recipes, the `prev_dir: str | Path`
positional argument is often getting `dict` entries thrown into, as a
result of `pw_copy_files` and such (see the test suite).

Because of this mess, things were in desperate need of a revamp, which
this PR addresses.

## User-Facing Changes

In most cases, there are no breaking changes to users and is mostly an
internal refactor. Some positional arguments of `prev_dir` have been
renamed `copy_files` for consistency with the type hint, although no
user changes are expected if used as positional. Some internal functions
have slightly modified behavior, such as `copy_decompress_files`. Refer
to the function signature for more.

---------

Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant