Ticket #2921 enhancement closed fixed

Opened 6 years ago

Last modified 6 years ago

twistd profiling options needs to be tested

Reported by: therve Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: exarkun Branch: branches/twistd-profiler-tests-2921-2
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

This is first step to accomplish #2571. The goal is to refactor the free functions in app.py into one object, and to deprecated the old functions.

Change History

1

  Changed 6 years ago by therve

  • branch set to branches/twistd-profiler-tests-2921
  • author set to therve

(In [21925]) Branching to 'twistd-profiler-tests-2921'

2

  Changed 6 years ago by therve

(In [21926])

  • Refactor the functions into one AppProfiler object
  • Add tests for this object
  • Deprecate old free functions
  • Spot a bug in hotshot with Python >= 2.5, and correct it

Refs #2921

3

  Changed 6 years ago by therve

(In [21927]) Add tests for deprecation.

Refs #2921

4

  Changed 6 years ago by therve

  • priority changed from normal to highest
  • owner therve deleted
  • keywords review added

This is ready to review. The nice thing is that the branch corrects a bug with hotshot and Python 2.5. I think this is a move in the good direction.

5

  Changed 6 years ago by exarkun

  • status changed from new to assigned
  • owner set to exarkun

6

follow-up: ↓ 7   Changed 6 years ago by exarkun

  • keywords review removed
  • owner changed from exarkun to therve
  • status changed from assigned to new
  • cc exarkun added

This looks like a decent step in the right direction. One way it could be improved further, I think, is to separate different profilers into different objects and give them different implementations of the runWhatever method, instead of doing flag checking and having a bunch of different methods on one class.

I don't think it would go wrong to clean up the twistd command line, either. twistd --profiler=hotshot, etc, would be a lot nicer than what we have now.

Clearly this isn't a thorough review. I wanted to get your thoughts before proceeding in any direction.

7

in reply to: ↑ 6   Changed 6 years ago by therve

Replying to exarkun:

This looks like a decent step in the right direction. One way it could be improved further, I think, is to separate different profilers into different objects and give them different implementations of the runWhatever method, instead of doing flag checking and having a bunch of different methods on one class. I don't think it would go wrong to clean up the twistd command line, either. twistd --profiler=hotshot, etc, would be a lot nicer than what we have now.

I think both points are linked. If we allow ourselves to change the command line options, we can have something smarter than flag checking. But if we're trying to have a backward compatible mechanism, I'm not sure it will ease things :). But having separate objects and options would definitely be cleaner.

8

  Changed 6 years ago by therve

(In [22267]) Break the profiling into different options, refactor tests.

Refs #2921

9

  Changed 6 years ago by therve

(In [22269]) Update man pages and copyrights.

Refs #2921

10

  Changed 6 years ago by therve

  • owner therve deleted
  • keywords review added

Alright, back to review.

11

  Changed 6 years ago by exarkun

  • owner set to therve
  • keywords review removed

trial has profiler command line options too. They should probably be the same as twistd's.

Some of the new tests fail if profile isn't importable. They should skip, like the hotshot tests skip, I guess. Here's the error output I get from trial twisted.test.test_twistd:

===============================================================================
[ERROR]: twisted.test.test_twistd.AppProfilingTestCase.test_hotshot

Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/trunk/twisted/test/test_twistd.py", line 327, in test_hotshot
    profiler.run(reactor)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/application/app.py", line 91, in run
    self._reportImportError("hotshot", e)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/application/app.py", line 49, in _reportImportError
    raise SystemExit(s)
exceptions.SystemExit: Failed to import module hotshot: No module named profile; please install the python-profiler package
This is most likely caused by your operating system not including
the module to it being non-free. Either do not use the option
--profile, or install the module; your operating system vendor
may provide it in a separate package.

===============================================================================
[ERROR]: twisted.test.test_twistd.AppProfilingTestCase.test_hotshot

Traceback (most recent call last):
Failure: exceptions.ImportError: No module named profile; please install the python-profiler package
===============================================================================
[ERROR]: twisted.test.test_twistd.AppProfilingTestCase.test_hotshotSaveStats

Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/trunk/twisted/test/test_twistd.py", line 351, in test_hotshotSaveStats
    profiler.run(reactor)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/application/app.py", line 91, in run
    self._reportImportError("hotshot", e)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/application/app.py", line 49, in _reportImportError
    raise SystemExit(s)
exceptions.SystemExit: Failed to import module hotshot: No module named profile; please install the python-profiler package
This is most likely caused by your operating system not including
the module to it being non-free. Either do not use the option
--profile, or install the module; your operating system vendor
may provide it in a separate package.

===============================================================================
[ERROR]: twisted.test.test_twistd.AppProfilingTestCase.test_hotshotSaveStats

Traceback (most recent call last):
Failure: exceptions.ImportError: No module named profile; please install the python-profiler package
-------------------------------------------------------------------------------

In the twistd man page (thanks for updating it!), I guess the copyright statement should match the one in most of the source files. I think the man pages were just missed when the source files were updated (r11450).

_reportImportError says the module to it, should say the module due to it. Thanks for updating the log.msg/log.deferr in that method, too. I think _why shouldn't be passed by keyword though (it's private, after all). It should be safe to just pass it as the second positional argument (if it isn't, we should fix it so it is).

ProfilerRunner.run should probably use try/finally for sys.stdout manipulation. An exception raised by print_stats will mess stuff up seriously as it is, I think. Not sure how to test this, not sure when print_stats raises an exception.

Same for HotshotRunner.run. Also, hasattr is evil. getattr with a default is better, or you could just try to get the stream and catch the AttributeError.

runReactorWithLogging is missing a docstring.

12

  Changed 6 years ago by therve

(In [22286]) Process review comments.

Refs #2921

13

  Changed 6 years ago by therve

  • owner therve deleted
  • keywords review added

Alright, I think I've fixed all the reported problems. The only thing I left over is trial modifications: for now it has its one thing, so we should open another ticket to use the same funtions (hopefully, it'll be easier once this in).

Back to review.

14

follow-up: ↓ 15   Changed 6 years ago by exarkun

  • keywords review removed
  • owner set to therve

runWithProfiler and runWithHotshot deprecations would probably be better if they pointed at ProfileRunner and HotshotRunner, respectively. AppProfiler seems like it is something that can probably go away after we drop support for all the deprecated ways to invoke this functionality. Also, aren't the assertions about the file in the deprecation warning wrong? It should be the file of the caller of runWith* which the deprecation warning points to, not app.py.

If a profiler is unimportable, the ImportError gets logged four times (it does in trunk too, I think). Maybe it would be appropriate to improve this just a bit in this branch. Deleting the traceback.print_exc call in _reportImportError gets rid of one of them pretty easily.

If I pass an invalid name to --profiler, I get an UnboundLocalError.

If I pass --savestats and don't specify a profiler, the profiler switches to hotshot, which is a bit surprising.

15

in reply to: ↑ 14   Changed 6 years ago by therve

  • owner therve deleted
  • keywords review added

Erm, sorry for the crappy job, I probably hit the review button a bit too quick. So thanks for the review :).

If I pass --savestats and don't specify a profiler, the profiler switches to hotshot, which is a bit surprising.

This is the current behavior on trunk (I think). It's also documented as the default value of the profiler option. Do you think it should change?

16

  Changed 6 years ago by exarkun

It's surprising, I think. It's also undocumented, and there are incompatible changes to the command line already in the branch, so maybe another such change is a good idea.

I think that really everyone just wants to use cProfile/lsprof and profile/hotshot are silly historical accidents, so it may not really matter much either way.

17

  Changed 6 years ago by exarkun

  • owner set to therve
  • keywords review removed

Ah, the incompatibility I thought I saw was due to the man page being corrected to document that --profile takes an argument. No actual behavior changed though, great.

In _BasicProfiler._reportImportError docstring, distribution should be distributions or Linux distributions or packagers.

In ProfileRunner.run docstring, under standard should be under the standard.

In HotshotRunner.run docstring, under hotshot should be under the hotshot.

In AppProfiler docstring, I'd say something like "Class which selects a specific profile runner based on configuration options." to make it explicit and clear what is being "managed".

ApplicationRunner should document its profiler instance attribute.

The comma in the docstring of AppProfilingTestCase.test_profile is extraneous.

AppProfilingTestCase.test_withoutProfile lets a lot of code run in between putting None into sys.modules and setting up the try/finally block.

The comma in the docstring of AppProfilingTestCase.test_hotshot is extraneous.

The epytext (or lack thereof) for the nothotshot option in the docstring of AppProfilingTestCase.test_nothotshotDeprecation is inconsistent with the way options are marked up in other docstrings. Also, produce in that docstring should be produces. The sentence would also read a bit more easily if "switching" and "on" were next to each other instead of spread out so much.

In the docstring for AppProfilingTestCase.test_hotshotPrintStatsError, "during the print of the stats" is awkward. I'd say "while printing the stats".

The rest looks great. Merge when the above is taken care of.

18

  Changed 6 years ago by therve

(In [22449]) Process review comments.

Refs #2921

19

  Changed 6 years ago by therve

  • status changed from new to closed
  • resolution set to fixed

(In [22450]) Merge twistd-profiler-tests-2921

Author: therve Reviewer: exarkun Fixes #2921

Refactor twistd profiling options to be more easily testable, and add tests for them. This refactor leads to a bug correction of hotshot under Python 2.5 and to some cleanups.

20

  Changed 6 years ago by therve

(In [22451]) Revert r22450: failing tests of debian system.

Refs #2921

21

  Changed 6 years ago by therve

  • status changed from closed to reopened
  • resolution fixed deleted

Debian slaves fail with this error:

  File "/srv/bb-slave/Run/slave/full2.4/Twisted/twisted/test/test_twistd.py", line 21, in ?
    import hotshot.stats
  File "/usr/lib/python2.4/hotshot/stats.py", line 11, in ?
    sys.exit(1)
exceptions.SystemExit: 1

22

  Changed 6 years ago by therve

(In [22453]) Try to fix the problem.

Refs #2921

23

  Changed 6 years ago by therve

  • status changed from reopened to new
  • keywords review added
  • owner therve deleted

I don't want to know what happened, that would make me sick.

24

  Changed 6 years ago by exarkun

  • owner set to therve
  • keywords review removed

I changed the comment near the exception handling code a bit. Don't read it too closely if you don't want to learn more about why this happened. :P

Since handling the exception from importing hotshot.stats was the only change, I think this can probably be re-merged now. One thing I noticed though is that a couple of the builders have a large number of conch failures for this branch. Was it created when some bad conch code was merged into trunk?

25

  Changed 6 years ago by therve

  • branch changed from branches/twistd-profiler-tests-2921 to branches/twistd-profiler-tests-2921-2

(In [22499]) Branching to 'twistd-profiler-tests-2921-2'

26

  Changed 6 years ago by therve

  • status changed from new to closed
  • resolution set to fixed

(In [22502]) Merge twistd-profiler-tests-2921

Author: therve Reviewer: exarkun Fixes #2921

Refactor twistd profiling options to be more easily testable, and add tests for them. This refactor leads to a bug correction of hotshot under Python 2.5 and to some cleanups.

This remerge is due to some insanity of the profiler being non-free under debian.

27

  Changed 3 years ago by <automation>

  • owner therve deleted
Note: See TracTickets for help on using tickets.