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

Status badges

Description (last modified by mkoeppe)

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

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

Change History (26)

comment:1 Changed 2 years ago by chapoton

  • Cc vdelecroix jipilab cremona tscrim mmezzarobba mkoeppe added

comment:2 follow-up: Changed 2 years ago by chapoton

Adding the single line

gen = self.gen_embedding()

is enough to trigger the crash.

comment:3 Changed 2 years ago by chapoton

  • Priority changed from critical to blocker

This may be considered as a blocker, no ?

comment:4 Changed 2 years ago by gh-timokau

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 2 years ago by jipilab

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: Changed 2 years ago by chapoton

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

comment:7 Changed 2 years ago by chapoton

  • Description modified (diff)

comment:8 in reply to: ↑ 6 Changed 2 years ago by jipilab

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 2 years ago by chapoton

  • 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:

c6c70e7trac 28036 fix proposal

comment:10 Changed 2 years ago by embray

  • Milestone changed from sage-8.9 to sage-8.8

comment:11 Changed 2 years ago by chapoton

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

comment:12 follow-up: Changed 2 years ago by chapoton

  • 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:

fe00485another tentative fix
Last edited 2 years ago by chapoton (previous) (diff)

comment:13 Changed 2 years ago by mkoeppe

  • 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: Changed 2 years ago by mmezzarobba

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 mkoeppe

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 git

  • Commit changed from fe004857554f2323dd313de4f6c7d31f30e8e8d3 to c44fd16d84007e775df82d2671438f75d2da4324

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

c44fd16Use LazyFormat to fix infinite loop

comment:17 Changed 2 years ago by mkoeppe

  • 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 mkoeppe

  • Authors set to Matthias Koeppe
  • 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 mkoeppe

(Sorry for overwriting your branch.)

comment:20 Changed 2 years ago by chapoton

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

comment:21 follow-up: Changed 2 years ago by dimpase

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

comment:22 in reply to: ↑ 21 Changed 2 years ago by mkoeppe

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 2 years ago by chapoton

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 mkoeppe

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 vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

comment:26 Changed 2 years ago by vbraun

  • Branch changed from public/ticket/28036_v2 to c44fd16d84007e775df82d2671438f75d2da4324
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.