Right now the sps profiling feature is coded into the TTest._runTest method. We should move that code in his own file, to clearly separate concerns. See the discussion in bug 1190270 around this.
This move the sps_profile code in his own module. Works good locally, with and without --spsProfile option. pushed a try without sps here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dbe8cf64c09 Maybe we should push one try with sps also ?
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Attachment #8643314 - Flags: review?(jmaher)
Comment on attachment 8643314 [details] [diff] [review] sps_profile.patch Review of attachment 8643314 [details] [diff] [review]: ----------------------------------------------------------------- I like this patch in general, but I don't like the: from talos import utils from talos.utils import TalosError from talos.sps_profile import SpsProfile I didn't think we had talos as a module?
Attachment #8643314 - Flags: review?(jmaher) → review-
We have now, with bug 1189021. :)
I forgot! How will this work locally?
Just fine, since talos is installed (so the package talos exists locally), and "import talos" already worked there. This module thing was only an issue with harness, since we are not installing talos - but the dependencies on harness. But this has been fixed, so we can use real nice imports now.
does this imply that you run talos from a specific directory? I think it is ok, but I want to confirm before changing to a r+. Also have you done a push to try with --spsProfiling enabled?
(In reply to Joel Maher (:jmaher) from comment #6) > does this imply that you run talos from a specific directory? I think it is > ok, but I want to confirm before changing to a r+. Well, no, this is not required. Not for me at least, as I use the talos 'binary' that is provided. Also no just if the user uses a virtualenv (like what is done using INSTALL.py). So I think this is safe from that point of view. > > Also have you done a push to try with --spsProfiling enabled? Not yet! I was wondering what would be the appropriate syntax. Maybe exactly the one you provided in bug 1190270 ? Also the try here is pretty unreadable, because of the reasons explained by mail. The fix in bug 1190907 should do the trick for now, so I am waiting for it to be landed before repushing to try here - I think a bit with and whithout --spsProfiling (two trys).
Asking again for review here. Same patch as before, just a bit more of clean up. This is working file locally (missingsymbols.zip and profile_tresize.sps.zip are created when using --spsProfile and that MOZ_UPLOAD_DIR is defined), I'll push to try with sps_profile later.
Attachment #8644844 - Flags: review?(jmaher) → review+
Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e8adbff2ecb Same with sps profile option: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1a31131f27e
Oh, I tried to build the syntax manually for spsProfile, seems like a failure. Canceled, and repushed to try (using trychooser this time): https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb12c8fea1af
Hmm, looks pretty good but I'm a bit worried with the two red jobs in: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb12c8fea1af Does not look related to my change to me, but maybe I should try again the job without my change to be sure ?
I would try again, it could have been a bad base you started with.
Right, pushed to try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27e3de759a63
Joel, I have a bunch of errors that seems unrelated: send failed, graph server says: 11:20:19 INFO - RETURN:to determine geomean from 'test_run_values' for 50795710 - local variable 'values' referenced before assignment 11:20:19 INFO - RETURN: File "/var/www/html/graphs/server/pyfomatic/collect.py", line 295, in handleRequest 11:20:19 INFO - RETURN: geomean = valuesReader(databaseCursor, databaseModule, inputStream, metadata) 11:20:19 INFO - RETURN: File "/var/www/html/graphs/server/pyfomatic/collect.py", line 238, in valuesReader 11:20:19 INFO - RETURN: raise DatabaseException("to determine geomean from 'test_run_values' for %s - %s" % (metadata.test_run_id, str(x))) 11:20:19 INFO - RETURN: 11:20:19 INFO - RETURN: Also I still have two win8 talos chrome busted. Will retry those without my patch to be sure.
So tried without my patch, one busted, the other green. Will put back my patch an try again some more.
put pack my changes, on 4 jobs two busted, two success. It does not seems related to my change. I think we are good landing this, but maybe we should investigate what is this intermittent bug first ? As you want Joel - I would go to land that patch, since this does not seems related an that it only appears when the spsProfile option is activated on try (I suspect I tried again on a bad base as you suggested). Then we should try again with spsProfile, and maybe fill a bug.
lets land the patch. I suspect this is a specific test causing troubles.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.