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

Support parallelizing over collections that have non-Int lengths #28651

Merged
merged 6 commits into from
Aug 24, 2018
Merged

Support parallelizing over collections that have non-Int lengths #28651

merged 6 commits into from
Aug 24, 2018

Conversation

sbromberger
Copy link
Contributor

Changes splitrange to use Integer instead of Int so that mixed-width ints can be used. This fixes the following LightGraphs issue on 32-bit machines:

julia> g = Graph(10,20)
{10, 20} undirected simple Int32 graph

julia> Parallel.stress_centrality(g);

julia> g = Graph{Int64}(g)
{10, 20} undirected simple Int64 graph

julia> Parallel.stress_centrality(g);
ERROR: MethodError: no method matching splitrange(::Int64, ::Int32)
Closest candidates are:
  splitrange(::Int32, ::Int32) at /buildworker/worker/package_linuxarmv7l/build/usr/share/julia/stdlib/v1.0/Distributed/src/macros.jl:235

Changes `splitrange` to use `Integer` instead of `Int` so that mixed-width ints can be used. This fixes the following LightGraphs issue on 32-bit machines:
```
julia> g = Graph(10,20)
{10, 20} undirected simple Int32 graph

julia> Parallel.stress_centrality(g);

julia> g = Graph{Int64}(g)
{10, 20} undirected simple Int64 graph

julia> Parallel.stress_centrality(g);
ERROR: MethodError: no method matching splitrange(::Int64, ::Int32)
Closest candidates are:
  splitrange(::Int32, ::Int32) at /buildworker/worker/package_linuxarmv7l/build/usr/share/julia/stdlib/v1.0/Distributed/src/macros.jl:235
```
@sbromberger
Copy link
Contributor Author

sbromberger commented Aug 14, 2018

@pfitzseb on Slack (I agree wholeheartedly):

imho all functions there should use Integer or T where T<:Integer

@sbromberger
Copy link
Contributor Author

x-ref: sbromberger/LightGraphs.jl#985

@JeffBezanson
Copy link
Member

A slightly better fix is to change the calls to splitrange to splitrange(Int(N), ...). Otherwise splitrange will be unnecessarily specialized. At least, the np argument is always an Int and doesn't need to be changed.

@sbromberger sbromberger mentioned this pull request Aug 14, 2018
@sbromberger
Copy link
Contributor Author

@JeffBezanson - ref #28652. Thank you.

@sbromberger
Copy link
Contributor Author

sbromberger commented Aug 14, 2018

I'll leave it to someone else to decide, then. My #28652 alternative was closed (I disagree with the closure, but whatever). In any rate, I've fixed it locally.

@sbromberger sbromberger reopened this Aug 14, 2018
@sbromberger
Copy link
Contributor Author

Gah. Didn't mean to close this.

What path do we want to take?

update per @jeffbezansons' suggestions
@sbromberger
Copy link
Contributor Author

Ping on this - any chance it could be merged soon? This fixes LightGraphs on 32-bit machines.

Copy link
Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, would be great to have a test though.

@vchuravy vchuravy added parallelism Parallel or distributed computation needs tests Unit tests are required for this change backport pending 1.0 labels Aug 20, 2018
@KristofferC KristofferC mentioned this pull request Aug 20, 2018
@KristofferC
Copy link
Member

Bump for test and this can be merged.

@sbromberger
Copy link
Contributor Author

sbromberger commented Aug 23, 2018

Could someone else please write the test? I edited this in GitHub's editor and can't figure out how to add a different file to the change set.

@KristofferC
Copy link
Member

Check the branch out locally or go to https://github.com/sbromberger/julia/tree/patch-1 and edit with GitHub.

@mbauman mbauman removed the needs tests Unit tests are required for this change label Aug 23, 2018
@mbauman mbauman changed the title Update macros.jl Support parallelizing over collections that have non-Int lengths Aug 23, 2018
Moved to before `addprocs` since we don't need remote workers for the `splitrange` test.
@KristofferC
Copy link
Member

This test doesn't test what was implemented (and thus fails).

@@ -15,6 +15,11 @@ include(joinpath(Sys.BINDIR, "..", "share", "julia", "test", "testenv.jl"))
1
end

# PR #28651
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

for T in (Int64, Int128)
  n = @distributed (+) for i in Base.OneTo(T(10))
    i
  end
  @test n == 55
end

This only happens whenever length returns something that is not an Int which is relatively rare.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. I had something written out last night but yours is simpler. Thanks.

@sbromberger
Copy link
Contributor Author

This test doesn't test what was implemented (and thus fails).

Yup. The test was for the "old" fix. Sorry 'bout that. I've just pushed new tests (hat tip: @andreasnoack) that hopefully will work.

@vchuravy
Copy link
Member

Thanks Seth! Note to whoever merges this, please do a squash merge.

@sbromberger
Copy link
Contributor Author

please do a squash merge.

It'd be better for my reputation if you did a fixup instead, but I'll take a squash :)

@KristofferC
Copy link
Member

The commit messages can be modified when squashing so all traces of Github editing can be removed ;)

@sbromberger
Copy link
Contributor Author

Travis failure is OSX-only and seems to be unrelated:

The log length has exceeded the limit of 4 MB (this usually means that the test suite is raising the same exception over and over).

The job has been terminated

@vchuravy vchuravy merged commit 145224f into JuliaLang:master Aug 24, 2018
KristofferC pushed a commit that referenced this pull request Aug 25, 2018
@sbromberger sbromberger deleted the patch-1 branch August 25, 2018 15:55
KristofferC pushed a commit that referenced this pull request Sep 8, 2018
KristofferC pushed a commit that referenced this pull request Sep 8, 2018
@KristofferC KristofferC added bugfix This change fixes an existing bug and removed backport pending 1.0 labels Sep 27, 2018
KristofferC pushed a commit that referenced this pull request Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug parallelism Parallel or distributed computation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants