Skip to content

[Windows] Use multiprocessing.dummy on Windows for parallelism (plus adding Azure test pipeline.) #251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
seanyen wants to merge 3 commits into from
Closed

[Windows] Use multiprocessing.dummy on Windows for parallelism (plus adding Azure test pipeline.) #251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51718ed
Use threading + add Azure test pipeline.
seanyen Feb 9, 2019
8d2d2fb
fix.
seanyen Feb 9, 2019
ccc4e28
Fix.
seanyen Feb 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions azure-pipelines.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
jobs:
- job: 'Build'
pool:
vmImage: 'vs2017-win2016'
strategy:
matrix:
Python27:
python.version: '2.7'
Python35:
python.version: '3.5'
Python36:
python.version: '3.6'
steps:
- task: UsePythonVersion@0
inputs:
versionSpec: '$(python.version)'
architecture: 'x64'
- script: |
python -m pip install nose coverage argparse python-dateutil docutils pyparsing
python -m pip install flake8 flake8-blind-except flake8-builtins flake8-class-newline flake8-comprehensions flake8-deprecated flake8-docstrings flake8-import-order flake8-quotes
python -m pip install mock
- script: |
python -m nose --with-xunit
- task: PublishTestResults@2
condition: succeededOrFailed()
inputs:
testResultsFiles: '**/*.xml'
13 changes: 12 additions & 1 deletion src/catkin_pkg/packages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,24 @@

"""Library to find packages in the filesystem."""

import multiprocessing
import os

from .package import _get_package_xml
from .package import PACKAGE_MANIFEST_FILENAME
from .package import parse_package_string

if os.name == 'nt':
# https://docs.python.org/2/library/multiprocessing.html#windows
#
# On Windows, using multiprocessing requires the scripts to be
# "Safe importing of main module" to avoid RuntimeError.
# This restriction requires downstream tools to be modified to
# work. Instead, let's fallback to threading wrapper to get
# around it and still keep the parallelism on Windows.
import multiprocessing.dummy as multiprocessing
Copy link
Copy Markdown
Member

@dirk-thomas dirk-thomas Feb 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the impact of using threading for this on Windows? Is the performance still better than without?

Copy link
Copy Markdown
Contributor Author

@seanyen seanyen Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well... ran some benchmark tests today and in a test case where it has 1000 packages (and each has dummy 1000 <build_depend> nodes), the result looks not good for threading wrapper in general on Windows...

On CPython 2.7:

  • Disable parallel: 26 sec
  • Using multiprocessing: 7.5 sec
  • Using multiprocessing.dummy: 98.5 sec

On Anaconda Python 3.7:

  • Disable parallel: 12.8 sec
  • Using multiprocessing: 4.8 sec
  • Using multiprocessing.dummy: 14.5 sec

(My environment is using Intel® Xeon® Processor E5-1650 v4 6-cores CPU.)
(My test case is test_find_packages_with_large_amount_packages in https://github.com/seanyen-msft/catkin_pkg/blob/windows_port_multiprocessing_disabled/test/test_packages.py)

Copy link
Copy Markdown
Member

@dirk-thomas dirk-thomas Feb 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The results don't look like this should be merged as is.

Copy link
Copy Markdown
Contributor Author

@seanyen seanyen Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That make senses. Since now we explored some approaches, any pointers what to investigate next at this moment?

else:
import multiprocessing


def find_package_paths(basepath, exclude_paths=None, exclude_subspaces=False):
"""
Expand Down
4 changes: 2 additions & 2 deletions test/test_packages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@ def test_find_packages_allowing_duplicates_with_no_packages():
@in_temporary_directory
def test_find_packages_invalid_version():
version = ':{version}'
path = 'src/foo'
path = os.path.normpath('src/foo')
Comment thread
dirk-thomas marked this conversation as resolved.
_create_pkg_in_dir(path, version)
try:
find_packages(path.split('/')[0])
find_packages(path.split(os.sep)[0])
assert False, 'Must raise'
except InvalidPackage as e:
exception_message = str(e)
Expand Down