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

Implement greedy scheduler #17

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Implement greedy scheduler #17

merged 4 commits into from
Jan 31, 2024

Conversation

MasonProtter
Copy link
Member

@MasonProtter MasonProtter commented Jan 31, 2024

Implements #11

Like split=:scatter, this option is a bit scary to expose for tmapreduce because it also requires that the reducing op be commutative.

I almost wonder if we should do some sort of argument validation to detect if the user gave us a non-commutative reducing operator...

e.g.

julia> treduce(vcat, [:a, :b, :c, :d, :e, :f, :g])
7-element Vector{Symbol}:
 :a
 :b
 :c
 :d
 :e
 :f
 :g

julia> treduce(vcat, [:a, :b, :c, :d, :e, :f, :g], schedule=:greedy, init=Symbol[])
7-element Vector{Symbol}:
 :b
 :f
 :a
 :g
 :d
 :e
 :c

This is not a problem for tforeach, tmap(f, OutputElementType, A), or tmap!, but it is a concern for regular tmap, as well as tmapreduce, treducemap, and treduce.

One other potential problem is that this algorithm is prone to giving errors if you run it on a small array and don't provide an init argument:

julia> treduce(vcat, [:a, :b, :c, :d, :e, :f, :g], schedule=:greedy)
ERROR: TaskFailedException
Stacktrace:
[...]
    nested task error: MethodError: reducing over an empty collection is not allowed; consider supplying `init` to the reducer
    Stacktrace:
[...]

this is because we're doing

    ch = Channel{Tuple{eltype.(Arrs)...}}(0; spawn=true) do ch
        for args  zip(Arrs...)
            put!(ch, args)
        end
    end
    tasks = map(1:ntasks) do _
        @spawn mapreduce(op, ch; mapreduce_kwargs...) do args
            f(args...)
        end
    end

so if one task gets spawned after the channel gets emptied, then we're in trouble.

@carstenbauer
Copy link
Member

Like split=:scatter, this option is a bit scary to expose for tmapreduce because it also requires that the reducing op be commutative.

I almost wonder if we should do some sort of argument validation to detect if the user gave us a non-commutative reducing operator...

I think documenting this requirement clearly for these options should be fine. After all, you have to actively opt for them. As for testing the commutative property, I would think that that's hard (perhaps even impossible) to do in general and also can be costly.

@MasonProtter
Copy link
Member Author

Do you think the docstrings I put in are sufficiently clear?

@carstenbauer
Copy link
Member

I think so, yes. But I'll revisit them when I set up Documenter (today).

@MasonProtter MasonProtter merged commit c6f8f89 into master Jan 31, 2024
6 checks passed
@MasonProtter MasonProtter deleted the greed-is-good branch January 31, 2024 16:26
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.

2 participants