TimespanLogging: Remove worker Id Nesting #387
Conversation
lib/TimespanLogging/src/core.jl
Outdated
| if only_local | ||
| # Only return logs from current process | ||
| mls = get_state(ml) | ||
| return mls.consumer_logs |
There was a problem hiding this comment.
We need to make a copy of these logs and then empty each vector contained in mls.consumer_logs, as the API of get_logs! is to remove the old logs from mls.consumer_logs so that future calls only include new logs.
There was a problem hiding this comment.
okay will fix it
Co-authored-by: Julian Samaroo <[email protected]>
| # Only return logs from the current process | ||
| logs = Dict{Int, Dict{Symbol,Vector}}() | ||
| pid = myid() | ||
| logs[pid] = remotecall_fetch(pid, ml) do ml |
There was a problem hiding this comment.
Why are you remotecall_fetch-ing here? The worker calling get_logs! with only_local=true is by definition the same worker that you need to fetch logs from, so you don't need that level of indirection.
| sublogs | ||
| end | ||
| end | ||
| return logs |
There was a problem hiding this comment.
This is wrong, it's now returning logs with an extra layer of nesting.
| A = rand(Blocks(4, 4), 16, 16) | ||
| collect(ctx, A*A) |
There was a problem hiding this comment.
These lines need to go inside the loop, to ensure we're generating logs for each of only_local=true and only_local=false.
| for c in keys(logs[w]) | ||
| @test isempty(logs[w][c]) | ||
| end | ||
| @testset "Get logs for only_local = true and only_local = false" begin |
There was a problem hiding this comment.
This extra level of testset nesting isn't necessary.
Removed the extra level of nesting by worker ID (#379)
Output when only_local is true: logs are fetched only from the current process.
Logs can be directly fetched from consumer_logs since there is only one worker.
Output when only only_local is false: logs are fetched from all the workers
