Added data-structure WeakKeyIdDict #1872

mauro3 · 2023-01-20T19:27:14Z

As discussed with @HechtiDerLachs over email, here the PR for adding WeakKeyIdDicts. Feel free to tweak it.

History: This originated in JuliaLang/julia#28161 and transitioned to JuliaLang/julia#28182. Then subsequently I de-coupled it from Julia and made a PR in DataStructures.jl JuliaCollections/DataStructures.jl#402, which thus far has not been merged either.

HechtiDerLachs · 2023-01-20T20:53:11Z

Thank you very much! This looks very good to me!

Unfortunately, the tests fail. And it looks like what you added seems to really interfere with some other internals. On a first glance, I do not see what exactly is going on, but with the help of those people whose code has been broken here, we will manage.

Other than that: @fingolfin, @thofma : Do you agree on the places where the files and include statements have been put?

mauro3 · 2023-01-20T22:35:40Z

Yeah, that looks like a pretty weird error. There seems to be some odd interaction occurring between the WeakKeyDicts in Hecke and this here...

In https://github.com/mauro3/Oscar.jl/tree/m3/weakkeyid-dict-error there is a slightly more minimal reproducer. The line
https://github.com/mauro3/Oscar.jl/blob/81d01e3bd9d06f92a3801d8298252db46e96e4a5/test/DataStructures/weakkeyid_dict-test.jl#L12
then leads to the failure when executing
https://github.com/mauro3/Oscar.jl/blob/81d01e3bd9d06f92a3801d8298252db46e96e4a5/test/Groups/abelian_aut.jl#L56
(this is on my local Julia 1.8.3 install)

Not sure, but might be a Julia bug...
Alternatively, running this code or not could trigger some garbage collection leading to that weak-ref not being defined anymore. Albeit, sprinkling GC.gc() statements around did not reproduce the error.

HechtiDerLachs · 2023-01-23T08:48:17Z

I checked out this pr on my local machine (running on julia 1.8.3).

To me it looked like the error occured in line 58 (not 54) of test/Groups/abelian_aut.jl. However, for me the tests seem to just go through!

I have to say, though, that I added the two lines

    @show L.weak_vertices_rev[k]
    @show typeof(L.weak_vertices_rev[k])

just before the assertion in line 461 of src/GrpAb/Lattice.jl in Hecke.jl (since I was curious what these objects really are).

But removing the two lines again does not seem to affect the tests (apart from printing stuff): They are still running for me.

Edit: In order to add those two lines, I had to do ]dev Hecke. That might have made a difference; looking into it now.

HechtiDerLachs · 2023-01-23T09:00:34Z

Calling ]free Hecke was complaining:

ERROR: Unsatisfiable requirements detected for package Hecke [3e1990a7]:
 Hecke [3e1990a7] log:
 ├─possible versions are: 0.5.0-0.16.6 or uninstalled
 └─restricted to versions 0.16.9-0.16 by Oscar [f1435218] — no versions left
   └─Oscar [f1435218] log:
     ├─possible versions are: 0.11.3 or uninstalled
     └─Oscar [f1435218] is fixed to version 0.11.3-DEV

so I had to execute ]up. Calling ]free Hecke again after that succeeded.

Then, I was starting up Oscar on my local branch with this PR and test/Groups/abelian_aut.jl went through, again.

HechtiDerLachs · 2023-01-26T19:42:46Z

@thofma : Could you have a look at the tests? I was talking with @fieker today since @simonbrandhorst suggested that this might have been his code that's producing the trouble here. But @fieker said, it would have been most likely you. Can you explain why the @assert is there, why it is failing now and whether one can do something about it?

thofma · 2023-01-27T08:30:30Z

When I wrote that code I was not too familiar with the GC. There is probably some lock missing, but the code is a bit complicated. It is the end of the term so I am a bit short on time. Once the term is over, I can say/do more.

HechtiDerLachs · 2023-01-27T10:46:29Z

Ok. Thanks for having a look! Let's just not forget about this.

mauro3 · 2023-01-31T16:18:23Z

Also note, there is at least one bug in the PR as it stands: when a key gets GC'ed then the dict-entry is not dropped but lingers as a "nothing" key. I can look into fixing this, if interest is around.

fingolfin · 2023-01-31T17:05:33Z

Is this based on the code of WeakKeyDict? If so, which version? Because WeakKeyDict underwent some major revisions in the past few years, including some to fix GC issues, see e.g. JuliaLang/julia#38180

mauro3 · 2023-01-31T17:11:39Z

Based on code from 2018, see "History" in the first post.

thofma · 2023-02-16T09:00:11Z

I released a new Hecke with a fix and rebased this PR here. This seems to have fixed the problem. There are now new exciting errors with segmentation faults (https://github.com/oscar-system/Oscar.jl/actions/runs/4191718023/jobs/7267185929).

HechtiDerLachs · 2023-02-17T22:41:08Z

I released a new Hecke with a fix and rebased this PR here.

Thank you very much for this! Unfortunately, I will be gone for some weeks now and don't have time look into this. But I shall get back to it!

fingolfin · 2023-02-22T14:25:03Z

Actually, we could use this in Nemo and perhaps even in AA.

So, I wonder, why not add it to AA, next to our WeakValueDict? Anyone got objections to that?

thofma · 2023-02-22T14:40:30Z

Actually, we could use this in Nemo and perhaps even in AA.

So, I wonder, why not add it to AA, next to our WeakValueDict? Anyone got objections to that?

I like it!

HechtiDerLachs · 2023-03-20T11:53:47Z

Did this discussion already move to Nemo/AA, then?

thofma · 2023-03-20T11:58:26Z

No, nothing has changed since my comment here: #1872 (comment). (Not quite true. There is now also a merge conflict.)

HechtiDerLachs · 2023-03-20T12:12:25Z

I tried again and the current version seems to work well now for the particular setting I have in mind. Here's some sample code:

using Test

wkd = WeakKeyDict{AbsSpec, MatrixElem}()
P, _ = QQ["x", "y", "z"]
X = Spec(P)
U = hypersurface_complement(X, P[1])
wkd[X] = P[one(P); P[1]]
wkd[U] = OO(U)[one(OO(U)); gens(OO(U))[1]]

@test haskey(wkd, X)
@test haskey(wkd, U)

U = hypersurface_complement(X, P[1]) # override U
GC.gc() # Call the garbage collector

@test length(keys(wkd)) == 1

X = Spec(P) # override X
U = hypersurface_complement(X, P[1]) # override U again as the previous one still had a reference to the original X

GC.gc() # Call the garbage collector
@test isempty(keys(wkd))

Edit: Sorry, my bad. I was using WeakKeyDict instead of oscar.WeakKeyIdDict. Replacing that accordingly in the above code leads to errors.

HechtiDerLachs · 2023-03-20T12:23:53Z

Here's a new chunk of code against which we can measure:

using Test

wkd = Oscar.WeakKeyIdDict{AbsSpec, MatrixElem}()
P, _ = QQ["x", "y", "z"]
X = Spec(P)
U = hypersurface_complement(X, P[1])
wkd[X] = P[one(P); P[1]]
wkd[U] = OO(U)[one(OO(U)); gens(OO(U))[1]]

@test haskey(wkd, X)
@test haskey(wkd, U)

### Delete only one key
U = hypersurface_complement(X, P[1]) # override U
@test !haskey(wkd, U) # check that `==` is not used.
GC.gc() # Call the garbage collector

@test length(keys(wkd)) == 1 # Throws an error: The number of stored keys is 2.
wkd[X] # Does not finish because of some internal lock. 
wkd # Throws a typeassert error. If I remember correctly, the key `X` was just 
    # replaced by `nothing` of type `Nothing` instead of being garbage collected. 
    # However, I can not reproduce this error right now. 

### Delete all keys
X = Spec(P) # override X
U = hypersurface_complement(X, P[1]) # override U again as the previous one still had a reference to the original X

GC.gc() # Call the garbage collector
@test isempty(keys(wkd))

It still produces several errors as annotated above.

fingolfin · 2023-09-08T08:59:10Z

I have just rebased this (but did not change anything otherwise). I'll now sync it up with change made to WeakKeyDict in recent Julia versions

fingolfin · 2023-09-08T09:55:39Z

@mauro3 I've updated this implementation to match the latest WeakKeyDict implementation in Julia 1.6 and later, which is known to fix various GC issues. I also added a comment explaining why we have to use WeakRefForWeakDict(key) instead of key in a bunch of place, even though WeakKeyDict gets away with using key (instead of WeakRef(key)), because I stumbled over that, and I think it might be helpful for the next person who has to touch this code.

Let's see how it fares... We still may want to move this to AbstractAlgebra, though. But since we had issues precisely with the interaction with Oscar, I felt it would be good to first get it working here, before opening a new PR for AbstractAlgebra.

fingolfin · 2023-09-08T10:02:43Z

The example by @HechtiDerLachs still fails, there's clearly a bug there:

julia> length(keys(wkd))
2

julia> keys(wkd)
KeySet for a Oscar.WeakKeyIdDict{AbsSpec, MatrixElem}(...). Keys:
  Spec of multivariate polynomial ring

julia> collect(keys(wkd))
2-element Vector{AbsSpec}:
    Spec of multivariate polynomial ring
 #undef

I'll look into it.

This should fix various GC issues, and ensure feature parity

fingolfin · 2023-09-08T10:11:47Z

Ah, fixed. I forgot Base. in a few places. Now the example by @HechtiDerLachs works fine for me.

codecov · 2023-09-08T12:15:49Z

Codecov Report

Merging #1872 (e0e8747) into master (7cd1abd) will decrease coverage by 0.04%.
Report is 11 commits behind head on master.
The diff coverage is 53.28%.

@@            Coverage Diff             @@
##           master    #1872      +/-   ##
==========================================
- Coverage   73.39%   73.36%   -0.04%     
==========================================
  Files         451      452       +1     
  Lines       64172    64382     +210     
==========================================
+ Hits        47098    47232     +134     
- Misses      17074    17150      +76

Files Changed	Coverage
src/Oscar.jl	`ø`
src/DataStructures/weakkeyid_dict.jl	`53.28%`

📢 Thoughts on this report? Let us know!.

fingolfin · 2023-09-08T12:43:59Z

Nice, all tests pass (at least the ones that pass elsewhere; Julia nightly and 1.10 are as broken as ever).

I'll open a PR for AbstractAlgebra then.

fingolfin · 2023-09-08T12:56:48Z

See Nemocas/AbstractAlgebra.jl#1419

fingolfin · 2023-09-13T14:11:03Z

WeakKeyIdDict is now in AbstractAlgebra 0.32.0, which will soon be available in Oscar (after we also update Nemo, Hecke, Singular...)

thofma force-pushed the m3/weakkeyid-dict branch from e1dec88 to 2516363 Compare February 16, 2023 07:32

fingolfin mentioned this pull request Feb 22, 2023

Unify matrix space implementations, possibly get rid of them Nemocas/Nemo.jl#1379

Closed

HechtiDerLachs force-pushed the m3/weakkeyid-dict branch from 2516363 to 2ba00d2 Compare March 20, 2023 12:00

Added data-structure WeakKeyIdDict

ece6153

fingolfin force-pushed the m3/weakkeyid-dict branch from 7113b83 to ece6153 Compare September 8, 2023 08:58

Sync WeakKeyIdDict with latest Julia WeakKeyDict implementation

e0e8747

This should fix various GC issues, and ensure feature parity

fingolfin force-pushed the m3/weakkeyid-dict branch from 017e2c9 to e0e8747 Compare September 8, 2023 10:11

fingolfin mentioned this pull request Sep 8, 2023

Add WeakKeyIdDict Nemocas/AbstractAlgebra.jl#1419

Merged

fingolfin closed this Sep 13, 2023

fingolfin mentioned this pull request Apr 12, 2024

Use WeakKeyIdDicts.jl Nemocas/AbstractAlgebra.jl#1666

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added data-structure WeakKeyIdDict #1872

Added data-structure WeakKeyIdDict #1872

mauro3 commented Jan 20, 2023

HechtiDerLachs commented Jan 20, 2023

mauro3 commented Jan 20, 2023 •

edited

Loading

HechtiDerLachs commented Jan 23, 2023 •

edited

Loading

HechtiDerLachs commented Jan 23, 2023

HechtiDerLachs commented Jan 26, 2023

thofma commented Jan 27, 2023

HechtiDerLachs commented Jan 27, 2023

mauro3 commented Jan 31, 2023

fingolfin commented Jan 31, 2023

mauro3 commented Jan 31, 2023

thofma commented Feb 16, 2023

HechtiDerLachs commented Feb 17, 2023

fingolfin commented Feb 22, 2023

thofma commented Feb 22, 2023

HechtiDerLachs commented Mar 20, 2023

thofma commented Mar 20, 2023

HechtiDerLachs commented Mar 20, 2023 •

edited

Loading

HechtiDerLachs commented Mar 20, 2023 •

edited by fingolfin

Loading

fingolfin commented Sep 8, 2023

fingolfin commented Sep 8, 2023

fingolfin commented Sep 8, 2023

fingolfin commented Sep 8, 2023

codecov bot commented Sep 8, 2023

fingolfin commented Sep 8, 2023

fingolfin commented Sep 8, 2023

fingolfin commented Sep 13, 2023

Added data-structure WeakKeyIdDict #1872

Added data-structure WeakKeyIdDict #1872

Conversation

mauro3 commented Jan 20, 2023

HechtiDerLachs commented Jan 20, 2023

mauro3 commented Jan 20, 2023 • edited Loading

HechtiDerLachs commented Jan 23, 2023 • edited Loading

HechtiDerLachs commented Jan 23, 2023

HechtiDerLachs commented Jan 26, 2023

thofma commented Jan 27, 2023

HechtiDerLachs commented Jan 27, 2023

mauro3 commented Jan 31, 2023

fingolfin commented Jan 31, 2023

mauro3 commented Jan 31, 2023

thofma commented Feb 16, 2023

HechtiDerLachs commented Feb 17, 2023

fingolfin commented Feb 22, 2023

thofma commented Feb 22, 2023

HechtiDerLachs commented Mar 20, 2023

thofma commented Mar 20, 2023

HechtiDerLachs commented Mar 20, 2023 • edited Loading

HechtiDerLachs commented Mar 20, 2023 • edited by fingolfin Loading

fingolfin commented Sep 8, 2023

fingolfin commented Sep 8, 2023

fingolfin commented Sep 8, 2023

fingolfin commented Sep 8, 2023

codecov bot commented Sep 8, 2023

Codecov Report

fingolfin commented Sep 8, 2023

fingolfin commented Sep 8, 2023

fingolfin commented Sep 13, 2023

mauro3 commented Jan 20, 2023 •

edited

Loading

HechtiDerLachs commented Jan 23, 2023 •

edited

Loading

HechtiDerLachs commented Mar 20, 2023 •

edited

Loading

HechtiDerLachs commented Mar 20, 2023 •

edited by fingolfin

Loading