Opened 2 years ago

Last modified 12 months ago

#5824 enhancement new

Add automatic detection of the number of CPU core to trial -j

Reported by: therve Owned by: racheesingh
Priority: normal Milestone:
Component: trial Keywords:
Cc: jml, rachee.singh@… Branch:
Author: Launchpad Bug:

Description

It'd be nice if trial -j auto twisted detected the number of available cores and used that as the number of local workers.

Attachments (3)

ticket5824.patch (1.6 KB) - added by racheesingh 18 months ago.
Patch file for the proposed fix for Ticket #5824
ticket5824-round2.patch (2.6 KB) - added by racheesingh 18 months ago.
Proposed fix along with test case.
ticket5824-round3.patch (3.5 KB) - added by racheesingh 18 months ago.
Proposed fix along with test case, incorporating review comments.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 2 years ago by DefaultCC Plugin

  • Cc jml added

comment:2 Changed 18 months ago by itamar

  • Keywords easy added

The Python standard library multiprocessing module has a function that exposes the number of available CPUs.

comment:3 Changed 18 months ago by racheesingh

  • Owner set to racheesingh
  • Status changed from new to assigned

comment:4 Changed 18 months ago by racheesingh

I looked at the Python docs for multiprocessing. multiprocessing.cpu_count() seems like something we could use to get the number of CPUs. But the docs also mention this:

Return the number of CPUs in the system. May raise NotImplementedError.

Apparently, on Windows machines cpu_count may result in a NotImplementedError. So should we use this at all?

comment:5 Changed 18 months ago by racheesingh

Does this seem like a reasonable thing to do? [I tried out multiprocessing.cpu_count and modified the opt_jobs method like so.]

    def opt_jobs(self, number):
        """
        Number of local workers to run, a strictly positive integer.
        """
        # Case insensitive check if jobs was "auto"
        if number.lower() == "auto":
            try:
                number = multiprocessing.cpu_count()
            except NotImplementedError:
                # Windows might raise NotImplementedError while accessing cpu_count
                raise usage.UsageError(
                    "-j AUTO option not supported for your system.")
        else:
            try:
                number = int(number)
            except ValueError:
                raise usage.UsageError(
                    "Expecting integer argument to jobs, got '%s'" % number)
            if number <= 0:
                raise usage.UsageError(
                    "Argument to jobs must be a strictly positive integer")
        self["jobs"] = number

comment:6 Changed 18 months ago by racheesingh

  • Cc rachee.singh@… added

Changed 18 months ago by racheesingh

Patch file for the proposed fix for Ticket #5824

comment:7 Changed 18 months ago by racheesingh

  • Keywords review added
  • Owner racheesingh deleted
  • Status changed from assigned to new

comment:8 Changed 18 months ago by Julian

  • Keywords review removed
  • Owner set to racheesingh

Hey!

Thanks for working on this. The solution you submitted looks like it's headed in the right direction, but all patches submitted to Twisted have to have accompanying tests. Take a gander at http://twistedmatrix.com/trac/wiki/TwistedDevelopment if you haven't already. You'll need to test the new behavior that this ticket is proposing.

Thanks again for looking at this!

comment:9 Changed 18 months ago by racheesingh

Thanks for the pointers! I followed the links about how to add tests and I added a test in what seemed like the place where tests for trial located. After adding the test case I ran

 trial --testmodule twisted/scripts/trial.py

to check if my test case was running at all. But I found that it was not being run. Is there something I am missing here? I am attaching the patch file which has the test case I was trying to add.

Changed 18 months ago by racheesingh

Proposed fix along with test case.

comment:10 Changed 18 months ago by thijs

Thanks for your patch.

  1. you should run trial twisted.trial.test.test_script since you placed your test there
  2. the docstring should also mention it can raise a NotImplementedError
  3. or perhaps it should simply return 1 for platforms like win32 and print a warning instead?

comment:11 Changed 18 months ago by itamar

Thanks for that patch!

I'm not sure what thijs is referring to in items 2 and 3. Here are my review comments:

  1. "trial --help" should indicate auto is a possible option for --jobs, otherwise no one will know about the cool new feature you added!
  2. You should provide a test for the error case where NotImplementedError is raised.
  3. "-j AUTO option not supported for your system." would better read as "'-j auto option not supported for your system."`
  4. You should provide a news file (https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles) - "svn add" it and then "svn diff" will include it.

Please resubmit after addressing these issues.

comment:12 Changed 18 months ago by racheesingh

Thanks for the review comments. I have incorporated the comments from thijs and itamar and am attaching the diff. Please do tell me if I have missed something out (or misunderstood your feedback).

Changed 18 months ago by racheesingh

Proposed fix along with test case, incorporating review comments.

comment:13 Changed 18 months ago by racheesingh

  • Keywords review added
  • Owner racheesingh deleted

comment:14 Changed 16 months ago by jonathanj

  • Keywords review removed
  • Owner set to racheesingh

Thanks for working on this and your continued effort! I have a few review points:

  1. Please use two newlines around methods as per the Twisted Coding Standard.
  1. I think raising SkipTest, with an appropriate message, is better than passing the test in the case where NotImplementedError is raised.
  1. It seems like your test case should be split into two test cases: one testing that NotImplementedError causes the expected UsageError (without depending on multiprocessing.cpu_count to raise this exception) from parseOptions; and one testing (and potentially being skipped on platforms that do not support it) that the runner worker count is auto-detected.

comment:15 Changed 12 months ago by Julian

#6766 was marked as a duplicate of this.

comment:16 Changed 12 months ago by exarkun

Thanks for the work on this so far, everyone.

"trial --help" should indicate auto is a possible option for --jobs, otherwise no one will know about the cool new feature you added!

Also, there's a man page for trial. It should be updated as well.

comment:17 Changed 12 months ago by exarkun

  • Keywords easy removed
Note: See TracTickets for help on using tickets.