Some type annotations (1)

[hategan]

Description

This PR is a small part of an effort to systematically add type annotations to Parsl.

In its initial form, this triggers one type error, related to parsl/utils.py:112. Please see the comment before that and let's discuss what the solution should be.

Type of change

• Code maintentance/cleanup

[benclifford]

In its initial form, this triggers one type error, related to parsl/utils.py:112. Please see the comment before that and let's discuss what the solution should be.

That exception is only ever constructed with a string. I think the use of outputs in the init args is a cut-and-paste holdover from MissingOutputsException. So make the type into str? and maybe rename that parameter to message or something like that.

[benclifford]

I added a stub in mypy-stubs for @typeguard.typechecked annotations a while ago, because I wanted a typeguard decorated function to have the same type signature as the original pre-decorated function.

That code lives in mypy-stubs/typeguard.pyi

That feels to me similar-ish to what wrap_error is trying to do (but not in decorator form).

It looks like this:

from typing import TypeVar

Func = TypeVar('Func')

def typechecked(f: Func) -> Func: ...

[benclifford]

I had a bit of a play with this. One problem is that the wrapped function can return more than the original function - it might return a RemoteExceptionWrapper, rather than a value of the original returned type (so if the original return type was T, the new return type is Union[T, RemoteExceptionWrapper].

[hategan]

I was thinking that the typevar trick won't work since the wrapper returned by the decorator should have a proper signature. Bu then it looks like it's possible to simply not typecheck that part:

T = TypeVar('T')

def decorator(f: T) -> T:
        def w(*args, **kwargs):
                return f(*args, **kwargs)
        return w  # type: ignore

So I guess that and Union[T, ...] should work here.

[benclifford]

i went down a rathole reading about this and playing yesterday and the short answer seems to be no, the longer answer being "no, but the typed python community is slowly working towards it; you can write a hacky mypy plugin in the meantime".

This seems to be the big relevant issue on mypy:

python/mypy#3157

so if you don't figure out a pleasing solution, I guess add that issue into the TODO comment.

I was a bit concerned in my testing yesterday about how hard it was to get a type error to occur when I had got the types wrong in particular ways, so grr.

[benclifford]

I merged this into benc-mypy to see how well they played together - in principle they wouldn't have any typing conflicts aside from superficial ones, or ones arising from where I've made invasive changes to the code. And yeah, I'm happy with how they merged.

[hategan]

In its initial form, this triggers one type error, related to parsl/utils.py:112. Please see the comment before that and let's discuss what the solution should be.

That exception is only ever constructed with a string. I think the use of outputs in the init args is a cut-and-paste holdover from MissingOutputsException. So make the type into str? and maybe rename that parameter to message or something like that.

Seems rather deliberate to me. If you look at repr(), it is not a copy from MissingOutputExceptions, but, unlike that class, it indicates that _outputs is meant to contain a list.

Yadu?

[benclifford]

I dug into the history of BadStdStreamFile. When it was initially implemented, it did indeed take a list of files, like this:

raise pe.BadStdStreamFile([stdout, stderr], e)

but commit 376d798 in early 2018 changed that to invoke it like this, taking a single filename:

raise pe.BadStdStreamFile(stdout, e)

So the multi-file invocation went away at least 2 years ago, without renaming _outputs to singular form in the exception implementation (although changing the printed exception text).

[benclifford]

The codebase has conflicting invocations in that _outputs field though still - sometimes a human message and sometimes a filepath.

[hategan]

The codebase has conflicting invocations in that _outputs field though still - sometimes a human message and sometimes a filepath.

In both cases, it's a string. And repr is ambiguous enough to produce a reasonable result in either case.

[benclifford]

This is all fine except I'm not happy with the wrap_error typing still and thing given the complexity surrounding that, the typing for that should be removed from this PR.

[hategan]

This is all fine except I'm not happy with the wrap_error typing still and thing given the complexity surrounding that, the typing for that should be removed from this PR.

Ah, I actually missed that. It's the wrapper that changes the signature of the wrapped function, so yeah, my initial comment about this not being quite right stands and I got carried away by wishful thinking.

Callable[..., R] -> Callable[..., Union[R, ?Exception]] can at least provide some type information without seemingly causing problems. Thoughts?

[benclifford]

Callable[..., R] -> Callable[..., Union[R, ?Exception]] can at least provide some type information without seemingly causing problems. Thoughts?

If you can get something like that working, sure. I struggled for the short time I played with it.

[hategan]

Callable[..., R] -> Callable[..., Union[R, ?Exception]] can at least provide some type information without seemingly causing problems. Thoughts?

If you can get something like that working, sure. I struggled for the short time I played with it.

It works to the extent that the return type is correctly matched, including the Union[] part, but the args are treated as Any. So I think that's better than no typing at all. I'll commit shortly.