JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.75k stars 5.49k forks source link

CI: test failure in UUIDs with `uuid7(Xoshiro(0))` #55190

Closed jishnub closed 3 months ago

jishnub commented 3 months ago

Seen in https://buildkite.com/julialang/julia-master/builds/38290#0190cfa0-2041-4c0a-811a-2dd9bea0a58e

Error in testset UUIDs:
Test Failed at /cache/build/tester-amdci5-9/julialang/julia-master/julia-373bd8e992/share/julia/stdlib/v1.12/UUIDs/test/runtests.jl:61
  Expression: uuid7(Xoshiro(0)) == uuid7(Xoshiro(0))
   Evaluated: Base.UUID("0190cfc4-cab8-708f-918c-381a04770c92") == Base.UUID("0190cfc4-cab9-708f-918c-381a04770c92")
ERROR: LoadError: Test run finished with errors
in expression starting at /cache/build/tester-amdci5-9/julialang/julia-master/julia-373bd8e992/share/julia/test/runtests.jl:100
ERROR: A test has failed. Please submit a bug report (https://github.com/JuliaLang/julia/issues)
including error messages above and the output of versioninfo():
Julia Version 1.12.0-DEV.880
Commit 373bd8e992e (2024-07-20 10:10 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (i686-linux-gnu)
  CPU: 128 × AMD EPYC 7502 32-Core Processor
  WORD_SIZE: 32
  LLVM: libLLVM-17.0.6 (ORCJIT, znver2)
Threads: 1 default, 0 interactive, 1 GC (on 16 virtual cores)
Environment:
  JULIA_CPU_THREADS = 16
  JULIA_BINARYDIST_FILENAME =
  JULIA_SHELL = /bin/bash
  JULIA_VERSION = 1.12.0-DEV
  JULIA_IMAGE_THREADS = 16
  JULIA_NUM_THREADS = 1
  JULIA_INSTALL_DIR = julia-373bd8e992
  JULIA_TEST_VERBOSE_LOGS_DIR = /cache/build/tester-amdci5-9/julialang/julia-master/tmp/jl_kOolUC
  JULIA_CMD_FOR_TESTS = julia-373bd8e992/bin/julia .buildkite/utilities/timeout.jl julia-373bd8e992/bin/julia
  JULIA_TEST_IS_BASE_CI = true
  JULIA_TEST_MAXRSS_MB = 1536
  JULIA_CPU_TARGET = pentium4
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:44
 [2] runtests(tests::String; ncores::Int32, exit_on_error::Bool, revise::Bool, seed::Nothing)
   @ Base ./util.jl:703
 [3] top-level scope
   @ none:1
 [4] eval
   @ ./boot.jl:438 [inlined]
 [5] exec_options(opts::Base.JLOptions)
   @ Base ./client.jl:290
 [6] _start()
   @ Base ./client.jl:532
caused by: failed process: Process(setenv(`/cache/build/tester-amdci5-9/julialang/julia-master/julia-373bd8e992/bin/julia -C native -J/cache/build/tester-amdci5-9/julialang/julia-master/julia-373bd8e992/lib/julia/sys.so -g1 /cache/build/tester-amdci5-9/julialang/julia-master/julia-373bd8e992/bin/../share/julia/test/runtests.jl all --ci --skip Pkg Downloads`,["LONG_COMMIT=373bd8e992e9a4a8c5cd89e1efea16e2ffbb708b", "BUILDKITE_PIPELINE_NAME=julia-master", "BUILDKITE_AGENT_NAME=tester-amdci5.9", "JULIA_CPU_THREADS=16", "LANG=en_US.UTF-8", "JULIA_BINARYDIST_FILENAME=", "BUILDKITE_PLUGIN_EXTERNAL_BUILDKITE_VERSION=dea1b33fba992d460ffae69bce54f598330fb0d3", "BUILDKITE_SOCKETS_PATH=/root/.buildkite-agent/sockets", "BUILDKITE_GIT_CHECKOUT_FLAGS=-f", "BUILDKITE_LABEL=:linux: test i686-linux-gnu"  …  "BUILDKITE_BUILD_AUTHOR_EMAIL=jishnub.github@gmail.com", "BUILDKITE_PIPELINE_ID=53de1ea5-6dd3-4008-b976-161c9f34b0ad", "BUILDKITE_INITIAL_JOB_ID=0190cf9f-3b8d-4812-9df5-10a1c7c4dd7b", "SHELL=/shells/bash", "HOME=/root", "BUILDKITE_GIT_CLONE_MIRROR_FLAGS=-v", "BUILDKITE_STRICT_SINGLE_HOOKS=false", "SHORT_COMMIT=373bd8e992", "BUILDKITE_AGENT_DEBUG=false", "JOURNAL_STREAM=8:472527211"]), ProcessExited(1)) [1]
Stacktrace:
 [1] pipeline_error
   @ ./process.jl:598 [inlined]
 [2] run(::Cmd; wait::Bool)
   @ Base ./process.jl:513
 [3] run
   @ ./process.jl:510 [inlined]
 [4] runtests(tests::String; ncores::Int32, exit_on_error::Bool, revise::Bool, seed::Nothing)
   @ Base ./util.jl:695
 [5] top-level scope
   @ none:1
 [6] eval
   @ ./boot.jl:438 [inlined]
 [7] exec_options(opts::Base.JLOptions)
   @ Base ./client.jl:290
 [8] _start()
   @ Base ./client.jl:532

The difference in the two appears to be in the cab8 vs cab9 segments.

Seelengrab commented 3 months ago

Hmmm this'll be a bit tricky to fix properly :/ UUID7 has a time dependency as well, and there's no easy way to substitute that at the moment. It uses the timestamp with millisecond precision, and I would have expected these two calls to be close enough together for that to work out it CI. Unfortunate that it isn't, due to the highly parallel load on CI..

time() does a ccall under the hood, which apparently introduces enough jitter to make this test fail. We could disable this particular test, but that would also mean not testing that the RNG state is important. Alternatively, the internal function could be split so that the time component is added afterwards, and the RNG specific parts are tested seperately?

giordano commented 3 months ago

Alternatively, the internal function could be split so that the time component is added afterwards, and the RNG specific parts are tested seperately?

Yeah, defining an internal function which takes the time as input looks the easiest option (unless adding a timestamp argument to uuid7 is an option?)

StefanKarpinski commented 3 months ago

I would add the timestamp argument to uuid7. Seems like a reasonable API addition.

Seelengrab commented 3 months ago

The same problem would also exist with uuid1, since that also has a dependency on time() and an RNG. It's just not exposed due to that particular behavior not being tested, it seems.

I'm in principle open to adding a method to uuid7 (the timestamp is just milliseconds since the unix epoch), but for uuid1 that's a bit more complicated:

UUIDv1 is a time-based UUID featuring a 60-bit timestamp represented by Coordinated Universal Time (UTC) as a count of 100-nanosecond intervals since 00:00:00.00, 15 October 1582 (the date of Gregorian reform to the Christian calendar).

So if we want this, IMO the best way would be just taking in a Float64 (as if generated by time(), so UNIX time) and doing the required transforms internally ourselves.

Seelengrab commented 3 months ago

Looking at the tests again - turns out we're not even using the dedicated test vectors from the RFC :melting_face: I'll rework that in the coming week so that we can make use of those too.