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 alarm tests #39142

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Dec 16, 2024

As discussed in https://gist.github.com/user202729/52b0c7134ea34f78a4416cd19e28e578#checking-the-code-is-indeed-interruptable , the current doctest of SageMath that tests sage: alarm(0.5); f() only checks whether f can be interrupted within 10 minutes or whatever the doctest time limit is — which is not particularly useful.

With this change, if f doesn't get interrupted within 0.2 second of alarm() fired, a test failure will be reported.

📝 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 Dec 16, 2024

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

@user202729 user202729 force-pushed the fix-alarm-tests branch 2 times, most recently from cf96408 to 5b6e04d Compare December 16, 2024 13:44
@tobiasdiez
Copy link
Contributor

There are a few failing tests. Some of them, I've seen before (randomly, like the one in qqbar) but others seem to be new.

@user202729
Copy link
Contributor Author

user202729 commented Dec 18, 2024

On make-based build system, there's only the missing warning one.

Edit: this warning sometimes appear and sometimes not. The easiest way to test is probably to suppress the warning.

On conda… the only new one I can see is in doctest/util.py. It's about the function takes way too long compared to expected.

Edit: sometimes it takes shorter than expected instead https://github.com/sagemath/sage/actions/runs/12404238773/job/34629083816?pr=39142 . Need to investigate.

It is true that sometimes the alarm()/cancel_alarm() takes an unreasonably long time though, I can even reproduce that locally (surely it should only takes < 0.001s because it's all I/O, if it takes longer there's probably some sort of deadlock somewhere which is worth investigating.)

meson has been failing everywhere nowadays. (presumably #39139 which should be merged soon)


I got most of the tests to pass, except a few that uses Python time.sleep() on Mac. Looks like it's sort of a common issue https://travis-ci.community/t/sleep-functions-are-not-accurate-on-macos/6122

@user202729 user202729 marked this pull request as draft December 19, 2024 08:55
@user202729 user202729 marked this pull request as ready for review December 20, 2024 03:36
@user202729
Copy link
Contributor Author

user202729 commented Dec 20, 2024

This should be ready for review now.

The (fixed) test failures on Mac is weird, but my best guess is context switching (as explained in the comment).

The framework would be useful for e.g. #39075 (to determine how much buffer time is needed when test computer is too fast), so it would be helpful if someone can review this one quickly.

@tobiasdiez
Copy link
Contributor

tobiasdiez commented Dec 20, 2024

The changes look good to me. Do you have an idea where the (random?) timeout error in the ci is coming from?

@user202729
Copy link
Contributor Author

user202729 commented Dec 20, 2024

No idea actually. But then, before the patch alarm() has been spuriously failing sometimes (with nobody have any idea why it happens) so it's probably not caused by this PR.

This time around it is triggered by PyThread_acquire_lock_timed though (… longjmp out of Python function leads to deadlock on some non-reentrant lock…? Which sort of make sense because the function is not just a single C library function call)

2024-12-20T03:22:35.4891017Z sage -t --long --warn-long 30.0 --random-seed=286735480429121101562228604801325644303 src/sage/rings/polynomial/polynomial_element.pyx
2024-12-20T03:32:32.9307435Z ##[error]/sage/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/cysignals/signals.cpython-310-x86_64-linux-gnu.so(+0x93d4)[0x7fbcce8c53d4]
/sage/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/cysignals/signals.cpython-310-x86_64-linux-gnu.so(+0x9496)[0x7fbcce8c5496]
/sage/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/cysignals/signals.cpython-310-x86_64-linux-gnu.so(+0xc881)[0x7fbcce8c8881]
/lib/x86_64-linux-gnu/libc.so.6(+0x42520)[0x7fbccfe44520]
/lib/x86_64-linux-gnu/libc.so.6(+0x91117)[0x7fbccfe93117]
/lib/x86_64-linux-gnu/libc.so.6(+0x9cc78)[0x7fbccfe9ec78]
python3(PyThread_acquire_lock_timed+0xa0)[0x55ac6f158f00]
python3(+0x1a1151)[0x55ac6f1ad151]
python3(+0x18c549)[0x55ac6f198549]
python3(_PyEval_EvalFrameDefault+0x818)[0x55ac6f181ae8]

...

python3(+0x22cf02)[0x55ac6f238f02]
python3(_PyEval_EvalFrameDefault+0x6b6c)[0x55ac6f187e3c]
python3(_PyFunction_Vec
2024-12-20T03:32:32.9331746Z ----------------------------------------------------------------------
2024-12-20T03:32:32.9332946Z sage -t --long --warn-long 30.0 --random-seed=286735480429121101562228604801325644303 src/sage/rings/polynomial/polynomial_element.pyx  # Timed out
2024-12-20T03:32:32.9334003Z ----------------------------------------------------------------------

Also the random truncation.

@tobiasdiez
Copy link
Contributor

Okay, the same error indeed happens also elsewhere and I've opened to keep track of it #39183

Could you please add a sentence or two to the developer guide, otherwise it looks good to go from my side.

@user202729
Copy link
Contributor Author

user202729 commented Dec 22, 2024

@tobiasdiez The problem is the current best place to put it is https://cysignals.readthedocs.io/en/latest/sigadvanced.html so… need to ask e.g. @dimpase I suppose.

Just have 1-2 sentence mentioning the function name should be sufficient. I hope the docstring is descriptive enough.

I suggest

However, the test above may still pass if the function takes several seconds or minutes to be interrupted, as long as it does not timeout the doctest. For this purpose, SageMath provides the convenient wrapper sage.doctest.util.ensure_interruptible_after() context manager, which checks the code exits quickly after being interrupted, and also gives descriptive error message on test failure.

@tobiasdiez
Copy link
Contributor

Sorry, should have been more precise. I meant adding a short remark here https://doc.sagemath.org/html/en/developer/coding_basics.html#writing-testable-examples, explaining how to test that a method can be interrupted using the helper you introduced.

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.

2 participants