Opened 2 years ago
Closed 2 years ago
#28036 closed enhancement (fixed)
Fix infinite loop from #21161  repr of NumberFields (the parents) should indicate its embedding if there is one
Reported by:  chapoton  Owned by:  

Priority:  blocker  Milestone:  sage8.8 
Component:  number fields  Keywords:  
Cc:  embray, jdemeyer, vdelecroix, jipilab, cremona, tscrim, mmezzarobba, mkoeppe  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Volker Braun 
Report Upstream:  N/A  Work issues:  
Branch:  c44fd16 (Commits, GitHub, GitLab)  Commit:  c44fd16d84007e775df82d2671438f75d2da4324 
Dependencies:  Stopgaps: 
Description (last modified by )
Because this is causing some random infinite loops when running the testsuite. To reproduce, as reported in https://github.com/cschwan/sageongentoo/issues/541, use:
sage t long src/sage/structure/ src/sage/interfaces/
Another symptom:
The doctest
File "src/sage/structure/parent.pyx", line 1734, in sage.structure.parent.Parent.hom.register_embedding Failed example: K.coerce_embedding()(a)
when it does not fail, and then one calls
K.coerce_embedding()
again, makes sage crash.
Removing the change of repr made in #21161 fixes that.
For the complete log when the doctest fails, see for example
Change History (26)
comment:1 Changed 2 years ago by
 Cc vdelecroix jipilab cremona tscrim mmezzarobba mkoeppe added
comment:2 followup: ↓ 5 Changed 2 years ago by
comment:3 Changed 2 years ago by
 Priority changed from critical to blocker
This may be considered as a blocker, no ?
comment:4 Changed 2 years ago by
Thank you for analyzing this issue! Previous discussion in https://github.com/cschwan/sageongentoo/issues/541, were we assumed it was distro related.
I agree that it should be included in 8.8.
comment:5 in reply to: ↑ 2 Changed 2 years ago by
Replying to chapoton:
Adding the single line
gen = self.gen_embedding()is enough to trigger the crash.
I do not get a crash with 8.8.rc2... :S Hmm.
So there is no way to bypass the current issue and still be able to know via the repr
that the NumberField? is embedded? This is in itself problematic, right?
Reverting completely #21161 would be too bad... Isn't there any way to remove the crash caused by calling
gen = self.gen_embedding()
?
comment:6 followup: ↓ 8 Changed 2 years ago by
Nota Bene: I got the crash/not crash when using py3sage. Not tried with py2sage.
comment:7 Changed 2 years ago by
 Description modified (diff)
comment:8 in reply to: ↑ 6 Changed 2 years ago by
Replying to chapoton:
Nota Bene: I got the crash/not crash when using py3sage. Not tried with py2sage.
Hmm. I also tested with py3sage. But I might have some obscure side effects...
comment:9 Changed 2 years ago by
 Branch set to public/ticket/28036
 Commit set to c6c70e7ecac7582b7bdbba6c916cb77188d2eda2
Here is a branch that fixes the crash.. Not sure if this is the right thing to do. And no idea if this prevents the random infinite loop to reappear.
New commits:
c6c70e7  trac 28036 fix proposal

comment:10 Changed 2 years ago by
 Milestone changed from sage8.9 to sage8.8
comment:11 Changed 2 years ago by
the fix proposal provokes a few failing doctests, that may not be so simple..
comment:12 followup: ↓ 14 Changed 2 years ago by
 Branch changed from public/ticket/28036 to public/ticket/28036_v2
 Commit changed from c6c70e7ecac7582b7bdbba6c916cb77188d2eda2 to fe004857554f2323dd313de4f6c7d31f30e8e8d3
Here is another proposal.. (not really convincing either)
New commits:
fe00485  another tentative fix

comment:13 Changed 2 years ago by
 Description modified (diff)
 Summary changed from revert #21161 to revert #21161  repr of NumberFields (the parents) should indicate its embedding if there is one
comment:14 in reply to: ↑ 12 ; followup: ↓ 15 Changed 2 years ago by
Replying to chapoton:
Here is another proposal.. (not really convincing either)
Did you test it? I don't understand how it can work, since _embedding
is a cdef attribute. Apart from that, the idea looks sensible enough to me.
Another option may be to use a lazy string for the error message that causes the loop.
comment:15 in reply to: ↑ 14 Changed 2 years ago by
Replying to mmezzarobba:
Another option may be to use a lazy string for the error message that causes the loop.
+1
comment:16 Changed 2 years ago by
 Commit changed from fe004857554f2323dd313de4f6c7d31f30e8e8d3 to c44fd16d84007e775df82d2671438f75d2da4324
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c44fd16  Use LazyFormat to fix infinite loop

comment:17 Changed 2 years ago by
 Summary changed from revert #21161  repr of NumberFields (the parents) should indicate its embedding if there is one to Fix infinite loop from #21161  repr of NumberFields (the parents) should indicate its embedding if there is one
comment:18 Changed 2 years ago by
 Status changed from new to needs_review
I was able to reproduce this error. I agree it's a blocker. Using LazyFormat
seems to fix it for me. Please test.
comment:19 Changed 2 years ago by
(Sorry for overwriting your branch.)
comment:20 Changed 2 years ago by
This is a better solution indeed. Works fine for me (on py3sage).
comment:21 followup: ↓ 22 Changed 2 years ago by
can that 1line way to crash above be put into a doctest?
comment:22 in reply to: ↑ 21 Changed 2 years ago by
Replying to dimpase:
can that 1line way to crash above be put into a doctest?
I don't know how to reproduce the crash with a single doctest.
comment:23 Changed 2 years ago by
add one line
K.coerce_embedding()
after the line
File "src/sage/structure/parent.pyx", line 1734, in sage.structure.parent.Parent.hom.register_embedding Failed example: K.coerce_embedding()(a)
comment:24 Changed 2 years ago by
This triggers the error for me in an interactive session; but not if I add it to the doctest...
comment:25 Changed 2 years ago by
 Reviewers set to Volker Braun
 Status changed from needs_review to positive_review
comment:26 Changed 2 years ago by
 Branch changed from public/ticket/28036_v2 to c44fd16d84007e775df82d2671438f75d2da4324
 Resolution set to fixed
 Status changed from positive_review to closed
Adding the single line
is enough to trigger the crash.