Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix infinite loop from #21161 - repr of NumberFields (the parents) should indicate its embedding if there is one #28036

Closed
fchapoton opened this issue Jun 21, 2019 · 29 comments

Comments

@fchapoton
Copy link
Contributor

Because this is causing some random infinite loops when running the testsuite. To reproduce, as reported in cschwan/sage-on-gentoo#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

https://patchbot.sagemath.org/log/27408/Ubuntu/18.04/x86_64/4.15.0-52-generic/petitbonum/2019-06-20%2014:50:38?short

CC: @embray @jdemeyer @videlec @jplab @JohnCremona @tscrim @mezzarobba @mkoeppe

Component: number fields

Author: Matthias Koeppe

Branch/Commit: c44fd16

Reviewer: Volker Braun

Issue created by migration from https://trac.sagemath.org/ticket/28036

@fchapoton fchapoton added this to the sage-8.9 milestone Jun 21, 2019
@fchapoton
Copy link
Contributor Author

comment:2

Adding the single line

gen = self.gen_embedding()

is enough to trigger the crash.

@fchapoton
Copy link
Contributor Author

comment:3

This may be considered as a blocker, no ?

@timokau
Copy link
Contributor

timokau commented Jun 21, 2019

comment:4

Thank you for analyzing this issue! Previous discussion in cschwan/sage-on-gentoo#541, were we assumed it was distro related.

I agree that it should be included in 8.8.

@jplab
Copy link

jplab commented Jun 21, 2019

comment:5

Replying to @fchapoton:

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()

?

@fchapoton
Copy link
Contributor Author

comment:6

Nota Bene: I got the crash/not crash when using py3-sage. Not tried with py2-sage.

@fchapoton

This comment has been minimized.

@jplab
Copy link

jplab commented Jun 21, 2019

comment:8

Replying to @fchapoton:

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...

@fchapoton
Copy link
Contributor Author

comment:9

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:

c6c70e7trac 28036 fix proposal

@fchapoton
Copy link
Contributor Author

Branch: public/ticket/28036

@fchapoton
Copy link
Contributor Author

Commit: c6c70e7

@embray embray modified the milestones: sage-8.9, sage-8.8 Jun 21, 2019
@fchapoton
Copy link
Contributor Author

comment:11

the fix proposal provokes a few failing doctests, that may not be so simple..

@fchapoton
Copy link
Contributor Author

Changed branch from public/ticket/28036 to public/ticket/28036_v2

@fchapoton
Copy link
Contributor Author

Changed commit from c6c70e7 to fe00485

@fchapoton
Copy link
Contributor Author

comment:12

Here is another proposal.. (not really convincing either)


New commits:

fe00485another tentative fix

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title revert #21161 revert #21161 - repr of NumberFields (the parents) should indicate its embedding if there is one Jun 21, 2019
@mezzarobba
Copy link
Member

comment:14

Replying to @fchapoton:

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.

@mkoeppe
Copy link
Member

mkoeppe commented Jun 21, 2019

comment:15

Replying to @mezzarobba:

Another option may be to use a lazy string for the error message that causes the loop.

+1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 21, 2019

Changed commit from fe00485 to c44fd16

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 21, 2019

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c44fd16Use LazyFormat to fix infinite loop

@mkoeppe mkoeppe changed the title revert #21161 - repr of NumberFields (the parents) should indicate its embedding if there is one Fix infinite loop from #21161 - repr of NumberFields (the parents) should indicate its embedding if there is one Jun 21, 2019
@mkoeppe
Copy link
Member

mkoeppe commented Jun 21, 2019

Author: Matthias Koeppe

@mkoeppe
Copy link
Member

mkoeppe commented Jun 21, 2019

comment:18

I was able to reproduce this error. I agree it's a blocker. Using LazyFormat seems to fix it for me. Please test.

@mkoeppe
Copy link
Member

mkoeppe commented Jun 21, 2019

comment:19

(Sorry for overwriting your branch.)

@fchapoton
Copy link
Contributor Author

comment:20

This is a better solution indeed. Works fine for me (on py3-sage).

@dimpase
Copy link
Member

dimpase commented Jun 21, 2019

comment:21

can that 1-line way to crash above be put into a doctest?

@mkoeppe
Copy link
Member

mkoeppe commented Jun 22, 2019

comment:22

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.

@fchapoton
Copy link
Contributor Author

comment:23

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)

@mkoeppe
Copy link
Member

mkoeppe commented Jun 22, 2019

comment:24

This triggers the error for me in an interactive session; but not if I add it to the doctest...

@vbraun
Copy link
Member

vbraun commented Jun 22, 2019

Reviewer: Volker Braun

@vbraun
Copy link
Member

vbraun commented Jun 23, 2019

Changed branch from public/ticket/28036_v2 to c44fd16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants