This had to move from distributed since we'd get otherwise horrible circular imports

this is very similar to unpack_remotedata

TODO: Maybe this should inherit from WrappedKey or be replaced by it entirely.

For me, calling this class TaskRef would make more sense since it does reference a Task via its key.

I'm actually leaning towards nuking this class entirely in favor of WrappedKey (keeping that for backwards compat) but I'm happy to have TaskRef as an alias

todo: unused

How would I rename a result that was previously stored under a different key?

{"key": KeyRef("old-key")}?

yes, this

Note that I opted to keep this after all. It is not something end users (or even developers) really have to touch but it makes internals of the Task class much smoother. I'm not married to it, though, and could see it being removed down the line again. It would just require a lot more iterating over things, checking for types, etc.

Personally, I am fine adding some "boilerplate" structures if this makes things easier to understand. I like that these classes are different, this makes the things you have to consider in a single case easier

I'm not necessarily against these classes, I mostly dislike their name which suggests less magic than they actually perform. This might be rather something like a Sequence{Consolidation|Conflation|Combination|Comprehension|Reduction}Task

I don't feel like they are doing much magic and I'm not sure what kind of surprises are hidden here.
I don't find the names you suggested to help with any of this ambiguity.

I'm happy not to do anthing here, this can always be adjusted later should it remain a source of confusion.

There is an open question about public API surface.

I'm inclined to make this module private for now but export something like Task or WrappedKey top level but I don't have a strong opinion here.

+1

I like that this is explicit now

a key can be bytes??? or am I missing something here?

Theoretically, yes. This is a remnant of python2 compat where str was of type bytes and not unicode.

We should change this but I never put in the time to clean everything up. FWIW these two things needed to change

(and whatever pops up with mypy then).

I haven't used iskey here for perf reasons. The function dispatch is actually noticeable here

Thanks!

+1

Personally, I am fine adding some "boilerplate" structures if this makes things easier to understand. I like that these classes are different, this makes the things you have to consider in a single case easier

t.key not being in dsk means that we access a persisted result for example?

yes

Can you add a brief comment for this as well? Took me a while to come to that conclusion

This is protection if t.key is actually an input for a non-trivial task?

We would also catch a key that is an alias for 2 different results?

This behavior is tested in test_resolve_aliases

dsk = {
        "bar": "foo",
        "foo": (func, "a", "b"),
        "baz": "bar",
        "foo2": (func, "bar", "c"),
    }

will convert to

{
    'bar': Task('foo'), 
    'baz': Alias(bar), 
    'foo2': Task('foo2')
}

i.e. bar is pointing to foo and foo is only used by bar. Therefore, bar will just inherit the task (Now that I put this here, I notice that we should likely change the key of that Task object as well)

However, bar is being used in baz and foo2 so inlining it would require us to compute it twice.

Can you add a short comment, it took me a while to remember what exactly this was supposed to catch

Are Tasks never inlineable? Is this because we inline elsewhere?

this is because legacy and new don't play well with each other.

The convert_legacy_* can handle a mix of tasks but other layers cannot (so far, only distributed-only graphs come with Task objects to the conversion in distributed alone is sufficient. The very next step here should be to do a similar change in dask/dask to allow internals to assume we're working in the new system.

TLDR for now, legacy low level optimization is/should be disabled whenever a BaseTask is encountered.

For me, calling this class TaskRef would make more sense since it does reference a Task via its key.

What would actually happen in this self-referential case?

at the very least dask.order raises a "cycles found" exception because strictly speaking a graph with this kind of self reference is not a DAG.

We had similar logic in the distributed scheduler before so without this kind of filtering, nothing will work. It's just a question about where we do this

I'm not necessarily against these classes, I mostly dislike their name which suggests less magic than they actually perform. This might be rather something like a Sequence{Consolidation|Conflation|Combination|Comprehension|Reduction}Task

The Alias class feels slightly off. It's the only BaseTask class that does not return the key with which it's also registered - or at least, should be registered with - into dsk. Instead, it returns the aliased key. I think that the intention is to make it simple to skip the Alias, but we have to implement code for optimizing Aliases away nonetheless since users could also just explicitly use the key from dsk. This might be a pain we're facing because the existing API forces us into allowing Aliases. A more explicit variant would be something like Alias(old, new)

An earlier version distinguished those and I had very often Alias(key, key) objects which felt a little redundant but I think I can make it work. It also struck me as weird to have the key attribute allowed to be different.

Alias(key, ref=None) seems appropriate

I refactored this in 60df5d9

I used target for the variable name instead of ref or new (since new is not always true, sometimes this is a genuine reference and ref is already taken)

This also makes the resolve_aliases (function name is not ideal) code a little easier to read imo

Sounds good, target definitely works. I wanted to avoid something ambiguous like Alias(key, alias) where it's unclear what the original name is.

Note this cache is unbounded. I didn't want to use an LRU (also because the acutally usable class for this is in distributed and the functools cache, etc. is not good). Also it's unclear how large the LRU should be.

Ideally, this was a cache that would work on bytes size so we could use zict, etc. for this but I didn't want to deal with this complexity just yet.

So, I'm running into an issue on dask/distributed in the test test_threadsafe_get. The tests is launching 30 threads that are concurrently hammering the scheduler with the same computation request over and over again, i.e. the tokens should always be identical and the result should in most likeliness even be reused between all the threads.
However, I'm running into problems and this appears to fix it. Haven't managed to isolate it (already worked with RLocks in various places which didn't help). The problem seems to be a counting issue in the seen dictionary but I couldn't reduce it or write a test for it.

I'm slightly concerned about this fix since we call this function quite a lot and copying a non-empty set can be expensive (haven't done any measurements, yet)

I removed this again. A workaround for this is to use tokenize directly instead of normalize_token for the GraphNode classes. This way, the likelihood of having those nested self referencing constructs is much smaller / doesn't show up. It's also better from a performance perspective since normalize_token can generate ridiculously deeply nested structures.

Should we stay consistent and rename this to something like propagate_data? There are a few other places where the word literal is being used, we may want to check for consistency there as well.

Should we add an issue for this?