Opened 5 years ago

Last modified 4 years ago

#5824 enhancement new

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

Reported by: therve Owned by: Rachee Singh
Priority: normal Milestone:
Component: trial Keywords:
Cc: Jonathan Lange, Rachee Singh Branch:
Author:

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 Rachee Singh 5 years ago.
Patch file for the proposed fix for Ticket #5824
ticket5824-round2.patch (2.6 KB) - added by Rachee Singh 5 years ago.
Proposed fix along with test case.
ticket5824-round3.patch (3.5 KB) - added by Rachee Singh 5 years ago.
Proposed fix along with test case, incorporating review comments.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: Jonathan Lange added

comment:2 Changed 5 years ago by Itamar Turner-Trauring

Keywords: easy added

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

comment:3 Changed 5 years ago by Rachee Singh

Owner: set to Rachee Singh
Status: newassigned

comment:4 Changed 5 years ago by Rachee Singh

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 5 years ago by Rachee Singh

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 5 years ago by Rachee Singh

Cc: Rachee Singh added

Changed 5 years ago by Rachee Singh

Attachment: ticket5824.patch added

Patch file for the proposed fix for Ticket #5824

comment:7 Changed 5 years ago by Rachee Singh

Keywords: review added
Owner: Rachee Singh deleted
Status: assignednew

comment:8 Changed 5 years ago by Julian Berman

Keywords: review removed
Owner: set to Rachee Singh

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 5 years ago by Rachee Singh

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 5 years ago by Rachee Singh

Attachment: ticket5824-round2.patch added

Proposed fix along with test case.

comment:10 Changed 5 years ago by Thijs Triemstra

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 5 years ago by Itamar Turner-Trauring

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 5 years ago by Rachee Singh

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 5 years ago by Rachee Singh

Attachment: ticket5824-round3.patch added

Proposed fix along with test case, incorporating review comments.

comment:13 Changed 5 years ago by Rachee Singh

Keywords: review added
Owner: Rachee Singh deleted

comment:14 Changed 5 years ago by Jonathan Jacobs

Keywords: review removed
Owner: set to Rachee Singh

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 4 years ago by Julian Berman

#6766 was marked as a duplicate of this.

comment:16 Changed 4 years ago by Jean-Paul Calderone

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 4 years ago by Jean-Paul Calderone

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