Opened 3 years ago
Closed 3 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: | sage-8.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/sage-on-gentoo/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 3 years ago by
- Cc vdelecroix jipilab cremona tscrim mmezzarobba mkoeppe added
comment:2 follow-up: ↓ 5 Changed 3 years ago by
comment:3 Changed 3 years ago by
- Priority changed from critical to blocker
This may be considered as a blocker, no ?
comment:4 Changed 3 years ago by
Thank you for analyzing this issue! Previous discussion in https://github.com/cschwan/sage-on-gentoo/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 3 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 follow-up: ↓ 8 Changed 3 years ago by
Nota Bene: I got the crash/not crash when using py3-sage. Not tried with py2-sage.
comment:7 Changed 3 years ago by
- Description modified (diff)
comment:8 in reply to: ↑ 6 Changed 3 years ago by
Replying to chapoton:
Nota Bene: I got the crash/not crash when using py3-sage. Not tried with py2-sage.
Hmm. I also tested with py3-sage. But I might have some obscure side effects...
comment:9 Changed 3 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 re-appear.
New commits:
c6c70e7 | trac 28036 fix proposal
|
comment:10 Changed 3 years ago by
- Milestone changed from sage-8.9 to sage-8.8
comment:11 Changed 3 years ago by
the fix proposal provokes a few failing doctests, that may not be so simple..
comment:12 follow-up: ↓ 14 Changed 3 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 3 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 ; follow-up: ↓ 15 Changed 3 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 3 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 3 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 3 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 3 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 3 years ago by
(Sorry for overwriting your branch.)
comment:20 Changed 3 years ago by
This is a better solution indeed. Works fine for me (on py3-sage).
comment:21 follow-up: ↓ 22 Changed 3 years ago by
can that 1-line way to crash above be put into a doctest?
comment:22 in reply to: ↑ 21 Changed 3 years ago by
Replying to dimpase:
can that 1-line 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 3 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 3 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 3 years ago by
- Reviewers set to Volker Braun
- Status changed from needs_review to positive_review
comment:26 Changed 3 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.