-
-
Notifications
You must be signed in to change notification settings - Fork 80
AskSage should not match just any "100" in response header #490
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
Conversation
|
The motivation make sense, as does the code. Can you provide a very simple AskSage test problem with which the code can be tested. |
|
Some of Drew's problems in Contrib can be used for a test file, for example Contrib/CUNY/CityTech/Precalculus/setSequences_-_Geometric/geometric-seq-nth-term.pg. Enter an answer for the nth term in a sequence in all parts, and this issue is rather likely to occur. It is not guaranteed to occur though, so you might need to try a few times. |
|
By the way, you will need to check the very end of the debug output (Drew: why do you have debug enabled in this problem?). If the error does not occur it will say "success is 1", and if the error does occur it will say "ERROR in contacting sage server. Did you accept the terms of service by setting {accepted_tos=>'true'} in the askSage options?" |
taniwallach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the version of this patch for develop: #491
I confirm that before the patch, the suggested sample problem would frequently fail and report:
ERROR in contacting sage server. Did you accept the terms of service by
setting {accepted_tos=>'true'} in the askSage options?
IO.pm: ERROR trapped during JSON call to sage:
Unable to make a sage call to https://sagecell.sagemath.org/service. at [PG]/lib/WeBWorK/PG/IO.pm line 442
at [PG]/lib/WeBWorK/PG/IO.pm line 502
and that after the patch those errors are not occurring. The code looks fine.
|
Given that this is a fix for an issue with functionality/stability in the master branch - I think we should merge the hotfix in the near future. |
|
I am 100% in favor of merging this hotfix ASAP -- I would imagine that most AskSage problems are broken without it.
I don't see debug enabled on this problem? It is quite possible that I accidentally committed a problem that still had the debug on - if you find which one, let me know? (a cursory grep didn't turn up anything) |
|
I don't see the debug flag now either. I must have added it when I was testing things before. I know after I did some testing yesterday I ran |
|
I think this has sat here long enough. I am going to merge this later today, unless there are objections. |
I have been running into sporadic issues with AskSage problems. They seemed to randomly fail when trying to connect to the Sage Cell Server.
It turns out the response header gives plenty of opportunity to match "100":
So, I've tightened up the regex matching for the header to more accurately identify HTTP 100 and HTTP 200 responses.
I'm making a matching pull request for develop, but I think this should be merged now.