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

Widen setindex_shape_check method signature #13252

Merged
merged 1 commit into from
Sep 23, 2015
Merged

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Sep 21, 2015

This removes the restriction on setindex_shape_check that the index lengths
must all be Int, allowing data structures to return non-Int lengths (e.g.,
UInt(1):UInt(2) or big(1):big(2)). Fixes #13250.

This removes the restriction on setindex_shape_check that the index lengths
must all be Int, allowing data structures to return non-Int lengths (e.g.,
`UInt(1):UInt(2)` or `big(1):big(2)`).  Fixes #13250.
mbauman added a commit that referenced this pull request Sep 23, 2015
Widen setindex_shape_check method signature
@mbauman mbauman merged commit 806eeda into master Sep 23, 2015
@mbauman mbauman deleted the mb/unusual_lengths branch September 23, 2015 15:09
mbauman added a commit that referenced this pull request Sep 23, 2015
This removes the restriction on setindex_shape_check that the index lengths
must all be Int, allowing data structures to return non-Int lengths (e.g.,
`UInt(1):UInt(2)` or `big(1):big(2)`).  Fixes #13250.

(cherry picked from commit e5b3cc6)
ref #13252
@tkelman
Copy link
Contributor

tkelman commented Sep 24, 2015

I believe this may be causing reducedim test failures? I'm going to remove this from the backport list in the meantime until that can be sorted out.

http://buildbot.e.ip.saba.us:8010/builders/build_ubuntu14.04-x64/builds/2515/steps/shell_2/logs/stdio

@tkelman
Copy link
Contributor

tkelman commented Sep 25, 2015

Okay, using tests passing 10 times in a row as an approximation for "probably not broken," it looks like accusing this PR was wrong. #13279 is looking like a more likely culprit.

10 wasn't enough apparently, still collecting more runs

@tkelman tkelman mentioned this pull request Sep 27, 2015
@tkelman
Copy link
Contributor

tkelman commented Sep 29, 2015

Still seeing this intermittently on buildbots. Can sometimes take many repetitions of running the tests to make happen locally, so there's something nondeterministic about it. I'm reasonably sure it was caused by this, unless there was something slightly earlier that also could have changed anything about reducedim?

    From worker 3:       * reducedim            Error During Test
    From worker 3:    Test threw an exception of type BoundsError
    From worker 3:    Expression: sum($(Expr(:typed_vcat, :Real, :(1 2 3), :(4 5.3 7.1))),2) == reshape([6,16.4],2,1)
    From worker 3:    BoundsError
    From worker 3:     in _mapreducedim! at reducedim.jl:206
    From worker 3:     in sum at reducedim.jl:264
    From worker 3:     in anonymous at test.jl:158
    From worker 3:     in do_test at test.jl:179
    From worker 3:     in include_string at loading.jl:266
    From worker 3:     in include_from_node1 at ./loading.jl:307
    From worker 3:     [inlined code] from util.jl:179
    From worker 3:     in runtests at /home/ubuntu/buildbot/slave/build_ubuntu14_04-x64/build/test/testdefs.jl:6
    From worker 3:     in anonymous at multi.jl:892
    From worker 3:     in run_work_thunk at multi.jl:645
    From worker 3:     [inlined code] from multi.jl:892
    From worker 3:     in anonymous at task.jl:59

@mbauman
Copy link
Member Author

mbauman commented Sep 30, 2015

I'm at a loss here. I cannot see any way in which this should change generated code… at all. So I can't even form any theories as to what might be going on. We could blindly try changing the signature to something like ::Integer, but how frequently would you say you see this occur, Tony? One in 10-20 times?

@tkelman
Copy link
Contributor

tkelman commented Sep 30, 2015

In the build range from 2515 (first failure on this buildbot) to 2552 (last successful build, that buildbot needs ssl-dev installed to pass after the libgit2 merge) http://buildbot.e.ip.saba.us:8010/builders/build_ubuntu14.04-x64?numbuilds=100, we had 4 instances of this failure

http://buildbot.e.ip.saba.us:8010/builders/build_ubuntu14.04-x64/builds/2515/steps/shell_2/logs/stdio
http://buildbot.e.ip.saba.us:8010/builders/build_ubuntu14.04-x64/builds/2517/steps/shell_2/logs/stdio
http://buildbot.e.ip.saba.us:8010/builders/build_ubuntu14.04-x64/builds/2526/steps/shell_2/logs/stdio
http://buildbot.e.ip.saba.us:8010/builders/build_ubuntu14.04-x64/builds/2533/steps/shell_2/logs/stdio

This buildbot shows slightly more: http://buildbot.e.ip.saba.us:8010/builders/build_centos7.1-x64?numbuilds=100 (and a few unrelated failures). I'm not positive whether it was this change that caused the problem, but it started happening very close to the time this was merged.

@GunnarFarneback
Copy link
Contributor

I can see no reason for this change to cause non-deterministic test failures. Possibly it could somehow uncover some other latent problem. Whatever is causing the test failure would be good to track down though, but so far I've not been able to reproduce it locally.

The actual failure is weird (well, maybe to be expected when it's non-deterministic). If I follow the traces and code correctly the failing test case is line 127 in test/reducedim.jl

@test sum(Real[1 2 3; 4 5.3 7.1], 2) == reshape([6, 16.4], 2, 1)

When it reaches _mapreducedim! it should have

f = IdFun()
op = AddFun()
A = Real[1 2 3; 4 5.3 7.1]
R = reducedim_init(f, op, A, 2)

where

julia> R = Base.reducedim_init(f, op, A, 2)
2x1 Array{Real,2}:
 0
 0

Since this makes size(R, 1) == 2 it shouldn't even enter the branch containing line 206, where the bounds error is reported. Apparently something goes wrong along the way but until I manage to reproduce the problem I won't be able to get farther.

@tkelman
Copy link
Contributor

tkelman commented Oct 4, 2015

I am confused immensely, and may have been wrong to blame this commit. The bad commit below reproduces the error within at most a few dozen runs, the good commit can run overnight repeatedly for a few days, probably 100 or more successes:

bad: [6b749dd] Move docsystem to end of sysimg.jl

git bisect bad 6b749dd

good: [b939592] 4-space indent in basedocs

git bisect good b939592

first bad commit: [6b749dd] Move docsystem to end of sysimg.jl

mbauman added a commit that referenced this pull request Oct 31, 2015
This removes the restriction on setindex_shape_check that the index lengths
must all be Int, allowing data structures to return non-Int lengths (e.g.,
`UInt(1):UInt(2)` or `big(1):big(2)`).  Fixes #13250.

(cherry picked from commit e5b3cc6)
ref #13252
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.

3 participants