https://pantsbuild.org/ logo
Powered by
h

hundreds-father-404

11/07/2019, 6:29 PM
Oof. I found the issue for why tests are taking so long now. It is NOT safe to use 
FrozenSet
. Even though it's immutable, it's not deterministic so messes up caching. https://github.com/pantsbuild/pants/pull/8577 changed 
PexRequirements
to use 
FrozenSet
instead of 
Tuple
, which means that resolve requirements are almost never cached now
a

average-vr-56795

11/07/2019, 6:30 PM
Good find!
👍 2
e

early-needle-54791

11/07/2019, 6:32 PM
oh wow
where is the indeterminism? In the way it stores set members?
h

hundreds-breakfast-49010

11/07/2019, 6:35 PM
Yeah I'm curious about this myself
and how exactly did you figure this out?
is it relatively easy to construct an example where the nondeterminism occurs?
h

hundreds-father-404

11/07/2019, 6:37 PM
Sets are always non-deterministic in Python, whereas Lists, Tuples, and Dicts are deterministic. I found this out by realizing how slow CI is taking for V2 and then running 
./pants --no-v1 --v2 test tests/python/pants_test/util:osutil
locally. It takes 60 seconds to resolve the requirements for that. On master, it will take the same amount of time. (I used simple 
print()
and 
time.time()
to measure this). With the PR I'm about to put up, the second time is less than a second
One thing I'm curious about is the CFFI boundary. How do we store sets in Rust?
e

early-needle-54791

11/07/2019, 6:38 PM
probably just Vecs
That might get converted to sets later
h

hundreds-father-404

11/07/2019, 6:40 PM
Is that how sets get converted from Rust->Python or from Python->Rust?
e

early-needle-54791

11/07/2019, 6:42 PM
Pretty sure that returns a pointer to a python object with the values of the rust set.
So rust would call that function to create a python set, and then do something with the handle to the python object
h

happy-kitchen-89482

11/07/2019, 7:08 PM
That is... bonkers
What do you mean by "deterministic" here?
a

average-vr-56795

11/07/2019, 7:09 PM
How the FFI happens depends on whether it’s going to be consumed from python or rust
If rust code is going to directly inspect the contents, the code @hundreds-father-404 linked to is used
h

happy-kitchen-89482

11/07/2019, 7:09 PM
even if the underlying representation changes, surely the values of hash and eq are consistent?
a

average-vr-56795

11/07/2019, 7:10 PM
If the thing is only going to be consumed from other python `@rule`s, what Henry described will happen
h

hundreds-father-404

11/07/2019, 7:11 PM
What do you mean by "deterministic" here?
How the data is stored. There's no guarantee that the ordering will be the same, even if 
__eq__
and 
__hash__
work This is how dictionaries worked with Python until 3.6
h

happy-kitchen-89482

11/07/2019, 7:14 PM
I mean, I want to say "of course there isn't". It's a set, not a list. But why does order matter?
If we're relying on iteration order on a set of any kind then that's a fundamental error.
It should be safe to cache on 
frozenset
as a value, just not on an iteration over its members.
h

hundreds-father-404

11/07/2019, 7:27 PM
It should be safe to cache on 
frozenset
as a value, just not on an iteration over its members.
I agree. I'm trying to find how to fix this and am not sure where we go from 
Python->Rust
. Changing 
native.py
didn't do anything
h

happy-kitchen-89482

11/07/2019, 7:29 PM
Basically, we should figure out why we're relying on order in a 
frozenset
, because that is a deep category error...
h

hundreds-father-404

11/07/2019, 7:34 PM
Basically, we should figure out why we're relying on order in a 
frozenset
, because that is a deep category error...
We probably just haven't special cased sets yet. Order does matter for other data structures: 
(1, "hello") != ("hello", 1)
for tuples.
h

happy-kitchen-89482

11/08/2019, 7:46 PM
Yes, that is by definition
My point is that all these data structures are well-defined and consistent if used as prescribed
👍 1
a tuple is order-preserving as part of its definition
a set is not
Powered by