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

Change Dict to be ordered by default #38145

Closed

Conversation

PallHaraldsson
Copy link
Contributor

@PallHaraldsson PallHaraldsson commented Oct 22, 2020

Fixes: #34265 Supersedes my #37804. See also my argument: #37761 (comment)

[EDIT: Based on @oxinabox code, OrderedDict from OrderedCollections.jl; as changing Dict to be ordered is unworkable for me, seems proven a breaking change, this PR is abandoned.]

@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Oct 22, 2020

I fixed non-serious docs failures, and this also went away, a false alam unrelated to my stuff(?):

cp: cannot stat 'dist-extras/7z.*': No such file or directory

@tpapp
Copy link
Contributor

tpapp commented Oct 23, 2020

As suggested in the discussion, please provide motivation for this. In particular, cf the discussions in #34265 and #10116.

Benchmarks would be interesting too.

@PallHaraldsson
Copy link
Contributor Author

The motivation is in the discussion you linked, and Jeff, Stefan and a lot of others want ordered by default, see this thread/comment:
#10116 (comment)

@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Oct 23, 2020

@JeffBezanson, this is probably ready for package eval, and it seems time sensitive for Julia 1.6.

I intentionally have Set unordered with older renamed Dict:

struct Set{T} <: AbstractSet{T}
    dict::UDict{T,Nothing}

It's something that could be changed later, seems not as important to me, unless you would want them in sync, both ordered (or not). I saw you can make some operation faster with ordered sets (while it wouldn't be with just Set changed to ordered).

EDIT2: I'll look into (while I've yet to see a connection to my change):

compiler/inference                 (5) |         failed at 2020-10-23T12:59:28.415
Test Failed at /buildworker/worker/tester_linux64/build/share/julia/test/compiler/inference.jl:1175
  Expression: Core.Compiler.invoke_api(linfo) == 2
   Evaluated: -1 == 2
Error During Test at /buildworker/worker/tester_linux64/build/share/julia/test/compiler/inference.jl:1201
  Expression evaluated to non-Boolean
  Expression: false || "Side effect expressions found $(ex)"
       Value: "Side effect expressions found Main.Test90Main_compiler_inference.fieldtype(Dict{Int64, Nothing}, :age)"
compiler/ssair                     (6) |     3.23 |   0.08 |  2.6 |     102.52 |   972.80
compiler/irpasses                  (6) |        started at 2020-10-23T12:59:28.547
compiler/irpasses                  (6) |     1.52 |   0.06 |  4.1 |     116.87 |   972.80
compiler/codegen                   (6) |        started at 2020-10-23T12:59:30.094
compiler/inline                    (8) |        started at 2020-10-23T12:59:38.034
compiler/codegen                   (6) |         failed at 2020-10-23T12:59:39.970
Test Failed at /buildworker/worker/tester_linux64/build/share/julia/test/compiler/codegen.jl:451
  Expression: success(`$(Base.julia_cmd()) --startup-file=no -e $str_36422`)
compiler/inline                    (8) |     1.83 |   0.02 |  1.3 |      94.67 |   310.91

EDIT: FYI, there are false alarms probably, but also:

dict                               (4) |         failed at 2020-10-23T13:01:44.303
Error During Test at /buildworker/worker/tester_linux64/build/share/julia/test/dict.jl:837
  Got exception outside of a @test
  Conversion to Dict is deprecated for unordered associative containers (in this case, WeakKeyDict{Any, Any}). Use an ordered or sorted associative type, such as SortedDict and Dict.
  Stacktrace:
    [1] depwarn(msg::String, funcsym::Symbol; force::Bool)
      @ Base ./deprecated.jl:82
    [2] depwarn(msg::String, funcsym::Symbol)
      @ Base ./deprecated.jl:80
    [3] convert(#unused#::Type{Dict{Any, Any}}, d::WeakKeyDict{Any, Any})
      @ Base ./ordered_dict.jl:92
    [4] macro expansion
      @ /buildworker/worker/tester_linux64/build/share/julia/test/dict.jl:847 [inlined]
    [5] macro expansion
      @ /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Test/src/Test.jl:1144 [inlined]
    [6] top-level scope
      @ /buildworker/worker/tester_linux64/build/share/julia/test/dict.jl:838
    [7] include
      @ ./Base.jl:393 [inlined]
    [8] macro expansion
      @ /buildworker/worker/tester_linux64/build/share/julia/test/testdefs.jl:24 [inlined]
    [9] macro expansion
      @ /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Test/src/Test.jl:1144 [inlined]
   [10] macro expansion
      @ /buildworker/worker/tester_linux64/build/share/julia/test/testdefs.jl:23 [inlined]
   [11] macro expansion
      @ ./timing.jl:343 [inlined]
   [12] runtests(name::String, path::String, isolate::Bool; seed::UInt128)
      @ Main /buildworker/worker/tester_linux64/build/share/julia/test/testdefs.jl:21
   [13] (::Distributed.var"#106#108"{Distributed.CallMsg{:call_fetch}})()
      @ Distributed /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Distributed/src/process_messages.jl:278
   [14] run_work_thunk(thunk::Distributed.var"#106#108"{Distributed.CallMsg{:call_fetch}}, print_error::Bool)
      @ Distributed /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Distributed/src/process_messages.jl:63
   [15] macro expansion
      @ /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Distributed/src/process_messages.jl:278 [inlined]
   [16] (::Distributed.var"#105#107"{Distributed.CallMsg{:call_fetch}, Distributed.MsgHeader, Sockets.TCPSocket})()
      @ Distributed ./task.jl:395
Test Failed at /buildworker/worker/tester_linux64/build/share/julia/test/dict.jl:1038
  Expression: pop!(d)
    Expected: ArgumentError
      Thrown: BoundsError
iobuffer                          (10) |        started at 2020-10-23T13:01:53.120

EDIT3:
https://build.julialang.org/#/builders/39/builds/4436
Will change Set back as, otherwise I get error for:
no method matching ht_keyindex2!(::Dict{Vector{Int64}, Nothing}, ::Vector{Int64})
no method matching skip_deleted_floor!(::Dict{Int64, Nothing})

Páll Haraldsson added 2 commits October 23, 2020 14:02
to try to silence test error. Change Set to, so no unordered used.
skip_deleted_floor! from the old Dict (now UDict), that no ordered dict
has and ht_keyindex2! (OrderedDict only has ht_keyindex2;
swissdict.jl does, but it's not ordered).
@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Oct 23, 2020

EDIT: At least one actual failure (but also in the original where I took the code from), giving stack-overflow: JuliaCollections/OrderedCollections.jl#65 I'm actually not too sure it's a bug in the recursive deepcopy test code, and if, if we want to keep bug-compatibility.

The 3 so-far buildbot/tester_* failures (and 3 more coming?) seem like false alarms, or something I do not understand, related to:

ps = _spawn(cmds, spawn_opts_inherit(args...))
success(ps) || pipeline_error(ps)

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.

Order of dict can change after serialization (make Dict ordered?)
3 participants