-
Notifications
You must be signed in to change notification settings - Fork 284
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
Added a timeout to stop requests.get hanging on platforms with no internet #3284
Added a timeout to stop requests.get hanging on platforms with no internet #3284
Conversation
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.
Thanks for this @wilbertcs ! A really helpful addition!
The SciTools-CLA-checker is failing as we don't have you on our list of known and approved contributors. Could you fill in this form https://scitools.org.uk/cla/v4/form which should (after a couple hours or so) automatically update the CLA-checker to pass
lib/iris/tests/__init__.py
Outdated
@@ -115,7 +115,9 @@ | |||
NC_TIME_AXIS_AVAILABLE = False | |||
|
|||
try: | |||
requests.get('https://github.com/SciTools/iris') | |||
# Added a timeout to stop the call to requests.get hanging on the HPC | |||
# which has restricted/no internet access. |
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.
"on the HPC" makes the comment a bit too specific to the Met Office. Could you make it more general and say "a platform with no internet access"
So something like:
# Added a timeout to stop the call to requests.get hanging when running
# on a platform which has restricted/ no internet access.
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.
Sorry to barge in, but just a couple of queries ..
- I think I understood that this is known to be Python3-specific problem ?
Are we sure of that / why that is / it is definitely appropriate to add an explicit timeout here (and fix does not belong here) - are we clear that 1 second is actually long enough ? It sounds possibly a bit short to me.
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.
No problem i'll update the message.
When testing this I found the cut off point to be about 0.6 seconds so chose 1 second as a timeout, however any advice on a more appropriate fix / timeout value would be appreciated.
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.
Our Travis runs don't always run with good resourcing, we frequently get test run timeouts where something that "normally" takes 5-6minutes stretches to 10-15.
So I'd say it's handy to be generous, maybe 5-10 seconds ?
I guess the downside of an over-early test failure would be, for us, if it silently misses a test which it should have run, and something unintended might slip through.
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.
Changed timeout to 10 seconds.
I've completed the CLA form as requested. |
I don't see the CLA checker. |
The commits are not identified as being owned by the user who signed the CLA. If these commits are yours @wilbertcs then you need to add the email address they were committed under to your Github account (Settings -> Emails) so that Github (and hence the CLA checker) will know they are yours. |
cd956e0
to
7be8dc5
Compare
Is it worth adding a flag constant ( |
Hi @mo-g! 👋 Good to hear from you :) I do want to get this merged before the 2.2.1 release, and for simplicity sake I think we should leave it as it is. But if it does prove to be an issue we could add it in a follow up PR |
@wilbertcs I'm not sure why the stickler-ci job isn't running. |
You too! :) |
Thanks for this @wilbertcs ! This will be available in the Iris 2.2.1 release, which I hope to cut in the next couple of weeks |
…ernet (SciTools#3284) * Added a 1 second timout to stop requests.get hanging on the HPC * Amended comment to be more generic * Changed 1 second timeout to 10 seconds.
…ernet (SciTools#3284) * Added a 1 second timout to stop requests.get hanging on the HPC * Amended comment to be more generic * Changed 1 second timeout to 10 seconds.
…ernet (SciTools#3284) * Added a 1 second timout to stop requests.get hanging on the HPC * Amended comment to be more generic * Changed 1 second timeout to 10 seconds.
When running our AT's ,we import 'IrisTest' from 'iris.tests'. Our tests run on the HPC which has restricted or no internet access, which causes the 'requests.get' call to hang indefinitely.
In this PR, I've added a 1 second timeout to the 'requests.get' call to stop the call hanging.
Testing on the HPC:
The following call will return with a 'ConnectTimeoutError' on the HPC :
'
'
which would allow the code to continue....
However , without the timeout the following call will hang indefinitely and has to be stopped, in this case, with a keyboardInterrupt:
`
`