Skip to content

adding -pthread for linux-ppc64le in setup.py - #8158

Merged
sklam merged 6 commits into
numba:mainfrom
esc:build/ppc64le/add_pthread_to_setup.py
Aug 12, 2022
Merged

adding -pthread for linux-ppc64le in setup.py#8158
sklam merged 6 commits into
numba:mainfrom
esc:build/ppc64le/add_pthread_to_setup.py

Conversation

@esc

@esc esc commented Jun 14, 2022

Copy link
Copy Markdown
Member

Fixes #8157

stuartarchibald
stuartarchibald reviewed Jun 14, 2022
View reviewed changes

@stuartarchibald stuartarchibald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the patch and for fixing this. Few comments inline, this also needs a buildfarm run and a manual test on a ppc64le machine, particularly against the threading layers.

Comment thread setup.py Outdated
Comment thread setup.py Outdated

ext_mviewbuf = Extension(name='numba.mviewbuf',
extra_link_args=install_name_tool_fixer,
extra_link_args=extra_link_args,

@stuartarchibald stuartarchibald Jun 14, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this library need pthreads?

@esc esc Jul 4, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

no, it appears as though it doesn't.

Comment thread setup.py
@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review Effort - medium Medium size effort needed and removed 3 - Ready for Review labels Jun 14, 2022
@esc

esc commented Jun 21, 2022

Copy link
Copy Markdown
Member Author

@stuartarchibald I have fixed up the items from the review and tested the patch on a ppc64le machine. The Numba test suite now passes on that platform w/o linking errors.

@esc esc added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Jun 21, 2022
@stuartarchibald

stuartarchibald commented Jun 22, 2022

Copy link
Copy Markdown
Contributor

@stuartarchibald I have fixed up the items from the review and tested the patch on a ppc64le machine. The Numba test suite now passes on that platform w/o linking errors.

Thanks @esc, does this need to be in the 0.56 milestone?

stuartarchibald
stuartarchibald reviewed Jun 22, 2022
View reviewed changes

@stuartarchibald stuartarchibald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the update, this looks about right and the proof is in the build on the PPC64LE architecture. There's a question inline but otherwise this seems ok. Thanks again!

Comment thread setup.py
depends=['numba/np/ufunc/workqueue.h'],
include_dirs=[os.path.join(tbb_root, 'include')],
extra_compile_args=cpp11flags,
extra_link_args=extra_link_args,

@stuartarchibald stuartarchibald Jun 22, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this needed for TBB pool but not OpenMP one? Am guessing the pthread_atfork call in TBB and no pthread.h API use in OpenMP (even though the header file is #included

numba/numba/np/ufunc/omppool.cpp

Line 21 in 9a7820c

#include <pthread.h>
probably from adding debug code during development, e.g. calls to pthread_self()).

@esc esc Jul 4, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, the OpenMP extension does not appear to need the linker flags.

@stuartarchibald stuartarchibald Jul 4, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for checking. I suggest removing the unused header as well as the use of the link time flags.

@esc esc Jul 21, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have removed the header in 5646137

Comment thread setup.py
@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jun 22, 2022
@esc esc added this to the Numba 0.57 RC milestone Jun 29, 2022
@stuartarchibald stuartarchibald self-assigned this Jul 12, 2022
esc added 3 commits July 20, 2022 12:21
@esc
include note about which c file needs pthreads
dd33b3d 
As title
@esc
Merge branch 'main' into build/ppc64le/add_pthread_to_setup.py
b57f404 
* main: (452 commits)
  Make numba.cuda.tests.doc_examples.ffi a module
  removed unused imports
  Clean up wrapper and only accept SUBPROC_TEST=1 as true.
  fix for /tmp/tmp access issues
  Fix links in `CONTRIBUTING.md`
  add a note to `numpy.concatenate`
  Respond to feedback, alter predicate to test os.environ directly.
  Add decorator to run a test in a subprocess.
  Add nrt.cpp to package distribution decl in setup.py.
  Replace atomic `.store()` with `operator=`
  Fix up cast to use definition directly.
  Respond to feedback from review.
  Updated docs
  Added documentation and removed fmsub distributions
  Flake8 fixes
  Remove use of mk_unique_var in inline_closurecall.py
  Fix typo in @vectorize docstring and a NumPy spelling.
  Respond to review comments:
  Added Advanced Distributions
  Update docs/source/reference/numpysupported.rst
  ...
@esc
OpenMP does not need pthreads
5646137 
As title
@esc esc added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Jul 27, 2022
@esc
fix typo
b17fe1a 
As title
stuartarchibald
stuartarchibald approved these changes Aug 12, 2022
View reviewed changes

@stuartarchibald stuartarchibald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the patch and fixes.

@esc esc added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Aug 12, 2022
@esc

esc commented Aug 12, 2022

Copy link
Copy Markdown
Member Author

@stuartarchibald thank you for the review. I double checked b17fe1a manually again on a power8 machine. I was able to compile Numba and the test suite ran to completion w/o issues.

@stuartarchibald

stuartarchibald commented Aug 12, 2022

Copy link
Copy Markdown
Contributor

@stuartarchibald thank you for the review. I double checked b17fe1a manually again on a power8 machine. I was able to compile Numba and the test suite ran to completion w/o issues.

Great, thanks for checking again @esc.

@sklam
sklam merged commit 2e9b58c into numba:main Aug 12, 2022
esc added a commit to esc/numba that referenced this pull request Aug 15, 2022
@esc
Revert "Merge pull request numba#8158 from esc/build/ppc64le/add_pthr…
19a3457 
…ead_to_setup.py"

This reverts commit 2e9b58c, reversing
changes made to 05e5e34.
@esc esc mentioned this pull request Aug 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stuartarchibald stuartarchibald stuartarchibald approved these changes
@sklam sklam Awaiting requested review from sklam

Assignees

@stuartarchibald stuartarchibald

Labels

5 - Ready to merge Review and testing done, is ready to merge Effort - medium Medium size effort needed

Projects

None yet

Milestone

Numba 0.57 RC

Development

Successfully merging this pull request may close these issues.

-pthread link flag missing in setup.py for linux-ppc64le

3 participants

@esc @stuartarchibald @sklam