Fix a very slow doctest in sage/data_structures/stream.py
#35127
Conversation
|
This is slightly worrisome to me because I recall testing this (repeatedly) and not noticing such a slowdown. Although it is purely in the plethysm code that is taking its time. Anyways, this is fine with me. It still tests what it needs to test. |
|
This is extremely strange. Could you check which commit caused the slowdown @saraedum ? |
Codecov ReportBase: 88.60% // Head: 88.58% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #35127 +/- ##
===========================================
- Coverage 88.60% 88.58% -0.02%
===========================================
Files 2136 2140 +4
Lines 396142 396961 +819
===========================================
+ Hits 350990 351663 +673
- Misses 45152 45298 +146
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
So, this is an extreme regression. I am very much against tuning done the doctest. In SageMath version 9.8.beta2, Release Date: 2022-10-16, testing the file takes about 1 second. |
|
9.8.beta7 takes 125.00s |
|
Ah, it might have been #27652. In sage 10.0.beta0: whereas In the following line, the left hand side is unaffected by the basis |
|
I am unable to see the suggestion. Can you post it directly here? From what I remember, when we want to propose an alternative fix, we are suppose to do a separate PR. Hopefully we will still be able to have a coherent discussion about the issue if we do that... |
|
OK, the suggestion is lost, I misunderstood github. I'll try again, differently. |
I guess it would be wise to keep the discussion on the issue page then. Or perhaps separate discussion of the issue from discussion of the specific implementation of the fix? |
This single test used to take ~ 200s, now it takes ~ 0.5s. Marked long time since the other 769 tests together take < 1s.
tornaria
force-pushed
the
slow_doctest-sage/data_structures/stream.py
branch
from
a3b1dcd
to
2bdb515
Compare
|
@tornaria, I'm afraid something didn't work out right. The forced push I see is the same fix as before. |
|
Ah, no, now I see, you introduced the |
You can comment in the review, also you can make small suggested changes that I can then apply to the PR. Another way is to do a PR against my branch in my repo then I can incorporate into this PR. In any case, this PR is just to fix the slow doctest and IMO should stay at that which is trivial to merge (and very important since some automatic testing fails due to timeout, If you want to fix the speed regression in the code (which would be great), I say make a separate PR. Doing that is outside my expertise. Edit: the run I referenced failed for a different reason, but I did get a timeout somewhere because of this. |
|
@tornaria, I don't get it. There is no code to fix. The fix is to introduce the additional |
Do you mean whether we can check this with asv benchmarks? asv supports this but we'd need to teach it how to rebuild sage after switching branches. That is not implemented currently. |
asv would have caught this, because it was a switch of convention in a method other than the method which was doctested, and the time went from 500ms to 120s on my computer. @tornaria, I'm not sure whether my other comment is visible: the reason I put in this doctest was to catch speed regressions, so I am very much against marking this as long. I am quite sure it would not have been noticed if it had the long marker before. |
The original test was fast when you introduced, but now it's slow. That's a speed regression. The test takes 0.6 - 1.0 s depending on the computer. That is long time. As I said in the review comment: Another option would be to always ran the test for range(5) (it's very fast) and then have a long time test to check 6 and 7 (and maybe 8). It all depends on what you are testing here (the If you want to test for speed regressions, I'm not sure this is the right place. Note that here you are doctesting The speed regression seems unrelated to Feel free to copy paste in a separate issue. |
|
@tornaria, I'm afraid I communicated not clearly enough. Let me say it again: there is no speed regression in the code. The parent of plethysm changed (see #27652), that's why adding the The policy on long doctests in sage is specified here: https://doc.sagemath.org/html/en/developer/doctesting.html#section-options, it clearly says "anything beyond about one second") This test takes less than half a second on my very ordinary computer. There are many tests in sage which take much longer. The reason I put in |
|
Documentation preview for this PR is ready! 🎉 |
|
Ping? |
|
@mantepse Are you suggesting to remove the addition of the The current PR keeps the test at |
|
@tscrim, I am very surprised. With the p inserted, the doctest takes less than half a second. Why would you mark it as long then? Yes, i am against adding the long. I know that there is currently no automatic tool that checks the timings. There is effort underway to change this, but it is not clear how well it will work. However, this doctest actually was detected (and quite early so) which should be a sign that it's well designed. |
|
It takes close to half a second on my fairly new laptop. So it isn't exactly "fast", and on slower machines, it might take longer than a second. This doctest would also have been detected even with being marked I don't see any harm in marking in |
|
Actually it wouldn't have been detected if it had had the long marker. It was detected because of a timeout. At least, I don't know of any mechanism that would have detected it as long. Moreover, i doubt it would have hit the timeout limit with 6 instead of 7. Finally, we have very many regular doctests which take roughly the same amount of time. Why is this one different, especially, since it serves a purpose? |
Are timeouts for long tests any different than for those not marked as long? I don't recall this being the case. It would have appeared from the I don't know for the present tester, but previously the patchbots ran I still do not see the harm in adding it.
I don't see the relevance of this here. It is about the duration of a doctest, not any specific one.
Not that many, and most that do don't already have other tests that cover them. Even if they are marked as |
|
If I understand correctly, the timeout was not within sage but elsewhere. In any case, neither the patchbot nor Volker noticed it. |
|
That said, if you are convinced that long is better, please go ahead. I don't see the reasons I gave addressed, but possibly this all becomes irrelevant once a proper benchmark tool is integrated. I do vaguely remember fixing a speed regression detected by findstat several times, and I must say that this is not only annoying but also time consuming. |
|
As I understand @tornaria, it occurred from one of the testers. I didn't see it on my desktop or laptop either, likely because they weren't slow enough to detect the timeout (on develop on my laptop): I understand what you want to protect against (and +1 for trying to find ways to detect it), but I don't think it will work at this point without someone manually noticing something. There are definite ways we could improve this, but (in principle) it should equally be detected by Indeed, finding speed regressions is a painful process (even more so in compiled code, which I had to do recently). Having a proper benchmarking tool will definitely be the way forward, if for nothing else to detect major regressions. We've tried a few times over the years as I recall, but getting a good robust framework has been the sticking point IIRC. Anyways, I should state that I am not opposed to adding the |
|
Some comments:
BTW, in my (not-shiny-and-new) server that I use for sage testing it takes more than 1 second: these two are the only two tests marked long time, everything else is very fast. |
|
@tornaria , would you have spotted the regression also if the test was marked as long? |
Yes, I run normal and long testsuite often. For the former I use You can also run the long testsuite when developing. It takes time but you don't have to run the whole thing, you could have a list of files that are influenced by code you care about, and run this more often when developing. As a starting point, use the output of |
|
Ok, then leave it as it is. I know how to run the tests, but I do not run the long ones outside of the module, because they take too long. I don't know why this regression was not noticed immediately when I fixed the semantics of plethysm.. neither did anybody else, except you. So i take your word that you will also catch similar regressions in the future. Apart from that , I'm sure that help on #35046 is highly welcome. |
It takes me 3 minutes to run all long tests in the 111 files that match the word
Because you didn't run the long tests outside the module. That's why I said that if you intend this test to catch regressions in this plethysm code, then it doesn't belong here. It seems better to place the test in the same module/function which might cause a failure. That being said, when changing semantics in some module, it seems very desirable to check thoroughly all users of the module, not just the module itself... For instance, it takes 3 minutes to run long tests in all 111 files. That's 180 seconds, so a single test taking 200 seconds does stand out (you are developing and you run tests often: if suddenly a single test takes 200 seconds this will piss you enough that you will spot the problem).
Again, I'd appreciate if you don't depend on me for this. Rather, figure out strategies to spot regressions early. Realize that once this test gets into a beta release, it will be run thousands of times and burden all CI. Worse for stable releases. I am still glad to catch and report stuff that slips through the cracks (there will always be some), but please do your best effort and learn from the things that escape your testing so that you catch similar issues next time.
|
|
Thank you for your great advice on how to develop code for sage. |
<!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes #1234" use "Introduce new method to calculate 1+1" --> ### 📚 Description This address the arity problem noted in #35261. It also removes a `TODO` in a `revert()` implementation by making the behavior the same across different implementations. Warnings are added for the user for the assumptions made by the code. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] I have made sure that the title is self-explanatory and the description concisely explains the PR. - [x] I have linked an issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open pull requests that this PR logically depends on --> - #35127 Avoiding a long test. - #35254 Avoiding merge conflicts. - #35291 Making sure these changes work together. - #35265 For some slight simplification of the new stream's `__getitem__`. URL: #35293 Reported by: Travis Scrimshaw Reviewer(s): Martin Rubey, Travis Scrimshaw
This single test used to take ~ 200s.
Now it takes ~ 1.5s and I marked it long time.
This was introduced in 94219d4 (#34413).
📚 Description
📝 Checklist
⌛ Dependencies