Closed
Bug 897067
Opened 11 years ago
Closed 11 years ago
"ImportError: cannot import name OrderedDict" at resourcemonitor.py line 14, on green mochitest runs
Categories
(Release Engineering :: General, defect)
Release Engineering
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: emorley, Assigned: emorley)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
1.99 KB,
patch
|
gps
:
review+
emorley
:
checked-in+
|
Details | Diff | Splinter Review |
A soon to be deployed TBPL parser improvement has found more uncaught Python exceptions in green Ubuntu debug mochitest runs.
a) Should these be making the test run fail?
b) We should catch/fix this so we don't spam the annotated failure summary once this TBPL patchset is rolled out.
eg:
https://tbpl-dev.allizom.org/php/getParsedLog.php?id=25605501&tree=Mozilla-Central#error2
Rev3 Fedora 12x64 mozilla-central debug test mochitest-browser-chrome on 2013-07-23 03:31:07 PDT for push fb4bf993a58a
slave: talos-r3-fed64-060
{
03:33:15 INFO - Running post-action listener: _resource_record_post_action
03:33:15 INFO - Running post-action listener: _start_resource_monitoring
03:33:15 WARNING - Unable to start resource monitor: Traceback (most recent call last):
03:33:15 WARNING - File "/home/cltbld/talos-slave/test/scripts/mozharness/base/python.py", line 388, in _start_resource_monitoring
03:33:15 WARNING - from mozsystemmonitor.resourcemonitor import SystemResourceMonitor
03:33:15 WARNING - File "/home/cltbld/talos-slave/test/build/venv/lib/python2.6/site-packages/mozsystemmonitor/resourcemonitor.py", line 14, in <module>
03:33:15 WARNING - from collections import (
03:33:15 WARNING - ImportError: cannot import name OrderedDict
}
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozsystemmonitor/mozsystemmonitor/resourcemonitor.py#14
14 from collections import (
15 OrderedDict,
16 namedtuple,
17 )
Flags: needinfo?(gps)
Assignee | ||
Comment 1•11 years ago
|
||
(Machines are running Python 2.6)
Comment 2•11 years ago
|
||
All automation machines should be running Python 2.7.3+.
Component: Mozbase → Release Engineering: Automation (General)
Flags: needinfo?(gps)
Product: Testing → mozilla.org
QA Contact: hskupin → catlee
Version: Trunk → other
Comment 3•11 years ago
|
||
Affects OSX 10.6 too.
https://tbpl.mozilla.org/php/getParsedLog.php?id=25976666&tree=Mozilla-Inbound
Comment 4•11 years ago
|
||
I'm going to push back that this is a bug with mozharness.
What's happening is we are attempting to start resource monitoring, catching an exception, and printing that exception to help with future diagnosis. The printing of that error is triggering the log parser.
The error is printed at warning level (as opposed to say error). The printed error has value, otherwise we wouldn't know why resource monitoring isn't working!
I think this is a bug with the log parser.
Assignee | ||
Comment 5•11 years ago
|
||
So this should be warning level ERROR and turning the run orange?
Comment 6•11 years ago
|
||
Not a bug in mozharness; it's getting marked as a WARNING.
TBPL is finding the assertion and highlighting it.
We could potentially add some magic string to each line that TBPL looks for first, to ignore errors, or hide the error.
Comment 7•11 years ago
|
||
(In reply to Ed Morley [:edmorley UTC+1] from comment #5)
> So this should be warning level ERROR and turning the run orange?
Nope, mozharness is appropriately marking it as a WARNING and not changing the end result of the run.
Comment 8•11 years ago
|
||
I think keeping the timestamp (to show that these errors are happening well before the real errors) and the log level would be helpful.
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
Updated•11 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Summary: "ImportError: cannot import name OrderedDict" at resourcemonitor.py line 14, on green Fedora debug mochitest runs → "ImportError: cannot import name OrderedDict" at resourcemonitor.py line 14, on green mochitest runs
Comment 9•11 years ago
|
||
Ed, I think we're going to have to just ignore this for now, as it doesn't appear a fix is coming any time soon.
Flags: needinfo?(emorley)
Assignee | ||
Comment 10•11 years ago
|
||
But surely if we want resource monitoring deployed & working - then it not being found is worthy of failing the run (hence what I meant by comment 5)...
Flags: needinfo?(emorley)
Assignee | ||
Comment 11•11 years ago
|
||
IMO we should be catching this exception if it is expected - hardcoding TBPL to ignore this failure seems extremely suboptimal...
Comment 12•11 years ago
|
||
RyanVM pinged me on IRC. I'm going to reiterate that I think this is a bug with the log parser. Well, foremost it's a bug with RelEng: they need to deploy Python 2.7 everywhere. If bug 711299 (opened in 2011!) were fixed, this problem would go away.
mozharness is trapping an error loading an *optional* feature and it is reporting that error to help with diagnosis so that failure can eventually be fixed. IMO it's doing everything right. If we failed to catch the error, resource monitoring would be backed out. If we didn't report the error, we wouldn't know why resource monitoring failed and would have a hard time making it work.
The problem is the downstream log parser is interpreting this caught error as a real error. It isn't a real error. Therefore the problem resides with the log parser.
A quick hack would be to have output that should be ignored by the error log parser to contain a token in the log line. e.g. perhaps we could prefix each log message with "optional" or some such. This is trivial to implement in mozharness. I think it should be relatively straightforward to adapt downstream log parsers as well. We just need to agree on the implementation and we can move forward.
Assignee | ||
Comment 13•11 years ago
|
||
We should still catch the exception and print a friendly message (eg: "Optional package not found!"), which would both be The Right Thing To Do & also fix the false positive in TBPL too :-)
Assignee | ||
Comment 14•11 years ago
|
||
(In reply to Ed Morley [:edmorley UTC+1] from comment #13)
> (eg: "Optional package not found!")
Well, more appropriately:
"Warning Python 2.7.x not installed, resource monitoring will not be enabled!"
Assignee | ||
Comment 15•11 years ago
|
||
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #791683 -
Flags: review?(gps)
Comment 17•11 years ago
|
||
Comment on attachment 791682 [details] [diff] [review]
Print useful error rather than exception
Review of attachment 791682 [details] [diff] [review]:
-----------------------------------------------------------------
This code is in mozbase and I'm not a peer, so I can't conduct a review. Please try :ahal or :jhammel if you wish to touch this file. See also https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Working_on_Mozbase_and_Contributing_Patches for their slightly-different patch landing process.
That being said, I would give this code an r- because continuing to load a module due to a missing required import should result in an Exception. We only ignore the failure to import psutil in resourcemonitor.py because psutil is optional - the resource monitor class can continue to function without it. Until Python 2.7 can be deployed everywhere (the proper solution to this bug), the temporary solution is for downstream consumers to trap the ImportError thrown by resourcemonitor.py. mozharness already does this, just not in a method the log parsers find acceptable.
I understand I'm not leaving you with an alternative. I think the least crappy solution is to catch the ImportError in mozharness.
Attachment #791682 -
Flags: review?(gps)
Comment 18•11 years ago
|
||
Comment on attachment 791683 [details] [diff] [review]
Fix most of the existing flake8 warnings
Review of attachment 791683 [details] [diff] [review]:
-----------------------------------------------------------------
Please follow the mozbase contribution guidelines to land this.
Also, I personally believe the subsequent line indenting is BS (wastes too much line real estate) and I don't follow it. I encourage you to do the same.
Attachment #791683 -
Flags: review?(gps)
Comment 19•11 years ago
|
||
Hi Ed, have you had a chance to work on this lately? :)
Flags: needinfo?(emorley)
Assignee | ||
Updated•11 years ago
|
Attachment #791683 -
Attachment is obsolete: true
Assignee | ||
Comment 20•11 years ago
|
||
Newer log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=28446601&tree=Mozilla-Central#error0
{
19:33:44 INFO - Running post-action listener: _resource_record_post_action
19:33:44 INFO - Running post-action listener: _start_resource_monitoring
19:33:44 WARNING - Unable to start resource monitor: Traceback (most recent call last):
19:33:44 WARNING - File "/home/cltbld/talos-slave/test/scripts/mozharness/base/python.py", line 433, in _start_resource_monitoring
19:33:44 WARNING - from mozsystemmonitor.resourcemonitor import SystemResourceMonitor
19:33:44 WARNING - File "/home/cltbld/talos-slave/test/build/venv/lib/python2.6/site-packages/mozsystemmonitor/resourcemonitor.py", line 14, in <module>
19:33:44 WARNING - from collections import (
19:33:44 WARNING - ImportError: cannot import name OrderedDict
}
Flags: needinfo?(emorley)
Assignee | ||
Updated•11 years ago
|
Attachment #791682 -
Attachment is obsolete: true
Assignee | ||
Comment 21•11 years ago
|
||
Don't attempt to start the (for now) optional resource monitor if Python < v2.7.
You're right, this is a much better place to handle this.
Attachment #811110 -
Flags: review?(gps)
Updated•11 years ago
|
Attachment #811110 -
Flags: review?(gps) → review+
Assignee | ||
Comment 22•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #811110 -
Flags: checked-in+
Assignee | ||
Comment 23•11 years ago
|
||
In production :-)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Component: General Automation → General
You need to log in
before you can comment on or make changes to this bug.
Description
•