Skip to content

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Sep 12, 2025

This pull request:

controlling this in bash seems easier than #39539 , for now. I suspect testing these files separately will make it stop failing however (doesn't really matter, the bug remains).

#40729 (comment) contains a traceback, but I think it isn't of too much help.

(Thought? Is --all --exclude=a --exclude=b better?)

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Sep 12, 2025

Documentation preview for this PR (built with commit 935cfc3; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit unfortunate that we have to resort to methods like this, but I guess it's still better than having failing runs all the time.

Can you please also directly include a few other files that are known to fail sometimes (just look for "random" or "flaky" in the issues). Otherwise looks good to go from my side.

@user202729
Copy link
Contributor Author

user202729 commented Sep 13, 2025

Can you please also directly include a few other files that are known to fail sometimes (just look for "random" or "flaky" in the issues).

these 2 files are special in that a fail induces segmentation fault. For other files, one can workaround them directly in Python, which has less overhead (only repeat that one test instead of all tests), and it's clearer at the source that it may fail.

As such, I don't want to lump them together.

@tobiasdiez
Copy link
Contributor

Fair enough, what about then at least still adding the following random seg faults:

@user202729 user202729 marked this pull request as draft September 13, 2025 13:51
@user202729
Copy link
Contributor Author

I don't think I've ever seen the giac one on CI though?

The matrix_gap might or might not be already fixed by #40613 / #40728 . (the latter is still work in progress for now…)

@tobiasdiez
Copy link
Contributor

okay, fine, we can also add them later if they indeed are still occurring on CI.

@user202729 user202729 marked this pull request as ready for review September 14, 2025 01:00
@orlitzky
Copy link
Contributor

Giac is optional now, largely because it's not portable. It shouldn't cause any problems with the CI. Mosts of the tests were moved into sagemath-giac.

Wouldn't it be simpler to delete the few tests that fail with hard-to-diagnose segfaults?

The point of an example is to show something that works, and these don't. The point of a test is usually to ensure that something keeps working, and these don't. The issues are tracked on github. If someone wants to work on them, it's a lot easier to find an open issue than it is to find a test that has been ignored with a special case in the doctesting framework, isn't it? Testing them and ignoring the result is just wasting cycles and complicating the CI / doctest runner.

@user202729
Copy link
Contributor Author

user202729 commented Sep 15, 2025

they're not just "a few test" as far as I can tell. If you delete that test, the next test that uses singular might as well fail… I think. (It's not easy to check whether it "sometimes" fail because it takes so much time)

Plus, there's already an open issue. (Several, in fact. They're correctly marked as duplicate though.)

@whoami730
Copy link
Contributor

@tobiasdiez
Copy link
Contributor

Wouldn't it be simpler to delete the few tests that fail with hard-to-diagnose segfaults?

The point of an example is to show something that works, and these don't. The point of a test is usually to ensure that something keeps working, and these don't. The issues are tracked on github. If someone wants to work on them, it's a lot easier to find an open issue than it is to find a test that has been ignored with a special case in the doctesting framework, isn't it? Testing them and ignoring the result is just wasting cycles and complicating the CI / doctest runner.

I agree, would also fix the issue that these test sometimes fail on developers systems. You also don't have to delete them, just add the known bug tag and they will no longer be executed. Last time I proposed changes in this direction (#36960) there was no interest in this, but I still think it's the cleanest approach.

@orlitzky
Copy link
Contributor

It really depends on whether or not singular becomes unusable like user202729 says, or if some individual tests can be disabled.

It looks like the problem occurs when we load the singular library freegb.lib under src/sage/algebras/letterplace, so skipping one test could indeed just cause it to segault on the next one that needs that library. But at the moment, freegb.lib is loaded in global scope. I guess we could try to load it on-demand, and then see if disabling the first failing test causes the next one to fail.

@tobiasdiez
Copy link
Contributor

I think all reported cases involve the g_algebra function and there are only a couple of calls in the codebase of this function. Moreover, not every call to g_algebra seems to be affected, at least I couldn't find reports about random failures at eg

sage: P.<x,y> = A.g_algebra(relations={y*x: -x*y}, order='lex')
.

In my opinion, this is another advantage of the known-bug-approach: we will know exactly which calls are triggering a certain issue, instead of "sometimes if the stars align in the wrong way something in this file throws a segfault".

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

Successfully merging this pull request may close these issues.

4 participants