add THREADING_LAYER_PRIORITY & NUMBA_THREADING_LAYER_PRIORITY by ickc · Pull Request #7234 · numba/numba

ickc · 2021-07-22T09:54:52Z

Reference an existing issue

esc · 2021-07-22T10:02:18Z

@ickc thank you for your submission and welcome to the Numba project! I have labeled your pull-request as "ready for review" and added it to the backlog. I have also labeled it as being a short review effort since the changes appear to be fairly straightforward. Note however, that we are currently in a release candidate phase (RC2 was published 19.07.2021) and so we are currently focused on any release critical bugs. This means it may potentially be some time until we can review it. We thank you in advance for you patience and understanding!

esc · 2021-07-22T10:23:42Z

@ickc do you want me to remove the "ready for review" label again?

ickc · 2021-07-22T10:36:24Z

@esc, do you think a test is needed? It seems that it should be residing in numba/tests/test_parallel_backend.py but I'm not sure how to test it as it is stateful.

I also want to test it locally first to make sure it is doing as claimed so may be removing the "ready for review" for now. Thanks.

esc · 2021-07-22T10:40:00Z

I also want to test it locally first to make sure it is doing as claimed so may be removing the "ready for review" for now. Thanks.

You got it! 👍

esc · 2021-07-22T10:41:33Z

@esc, do you think a test is needed? It seems that it should be residing in numba/tests/test_parallel_backend.py but I'm not sure how to test it as it is stateful.

Yes a test would be good. Also a test for the failure case and parsing the error message would be suitable. Not sure about the statefulness however.

in TestThreadingLayerPriority where parallel is not supported in 32-bit systems

stuartarchibald

Thanks for the PR. This is a good start and it's great to see this functionality added. I've given this an initial review, most comments are small things but there's a couple of more involved things in relation to testing. If anything is not clear please do ask :) Thanks again for working on this.

Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

to mention setting numba.config.THREADING_LAYER_PRIORITY

ickc · 2021-08-27T01:42:00Z

@stuartarchibald, ready for a 2nd round review.

stuartarchibald

Thanks for the update, few minor things to resolve else looks good.

stuartarchibald · 2021-09-10T16:08:58Z

+print(' '.join(numba.config.THREADING_LAYER_PRIORITY))
+print("@%s@" % numba.threading_layer(), file=sys.stderr)


Instead of using stdout/stderr, perhaps just make these into assertions such that the process will exit non-zero if there's a problem. The use of stdout/stderr relies on these streams never having unexpected things written to them (e.g. a deprecation warning) which makes their use more fragile.

The stdout should be safe. The stderr pattern @%s@ is actually copied from other tests in Numba (if you search this string in the same file there are 2 other such uses.) So I was just following whatever is practicing in Numba already...

The stdout should be safe. The stderr pattern @%s@ is actually copied from other tests in Numba (if you search this string in the same file there are 2 other such uses.) So I was just following whatever is practicing in Numba already...

This is one of the things the code base has been moving away from as it's a little error prone. As it's variables that are ending up as strings to test, having an assert on the actual variables is cheap and reduces maintenance burden. In the interests of getting this feature merged and pre-existing use of this pattern I'm included to leave it. I'll make a note of this pattern as something to refactor generally.

#7458 tracks.

stuartarchibald · 2021-09-10T16:09:47Z

@@ -306,6 +306,11 @@ def avx_default():
        DISABLE_JIT = _readenv("NUMBA_DISABLE_JIT", int, 0)

        # choose parallel backend to use


This comment does not match this variable, it belongs to THREADING_LAYER.

But shouldn't both THREADING_LAYER_PRIORITY and THREADING_LAYER are part of the "choose parallel backend to use" process?

I think that the THREADING_LAYER, if prescribed, will act as the choice for the parallel backend in use, whereas the THREADING_LAYER_PRIORITY sets preference for use of threading layers in the case that no explicit choice is prescribed?

Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

ickc · 2021-09-10T23:06:32Z

@stuartarchibald, thanks for the review. I resolved some trivial ones. For the 2 remaining open ones, I commented them and see if you agree with it.

stuartarchibald

Thanks for the patch and fixes.

add THREADING_LAYER_PRIORITY & NUMBA_THREADING_LAYER_PRIORITY

86a896e

esc added 3 - Ready for Review Effort - short Short size effort needed labels Jul 22, 2021

esc added 2 - In Progress and removed 3 - Ready for Review labels Jul 22, 2021

ickc added 4 commits July 22, 2021 14:36

add TestThreadingLayerPriority

b4d3909

fix test_invalid_env_var

2bb235a

fix flake8

c030b83

add skip_parfors_unsupported

b25abe8

in TestThreadingLayerPriority where parallel is not supported in 32-bit systems

ickc marked this pull request as ready for review July 23, 2021 03:02

ickc requested review from esc, sklam and stuartarchibald as code owners July 23, 2021 03:02

esc added 3 - Ready for Review and removed 2 - In Progress labels Jul 23, 2021

stuartarchibald reviewed Aug 3, 2021

View reviewed changes

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 Effort - short Short size effort needed labels Aug 3, 2021

ickc and others added 5 commits August 26, 2021 14:30

Update docs/source/reference/envvars.rst

328f4db

Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

Update docs/source/user/threading-layer.rst

8906558

Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

check len(namedbackends)

0377d0d

fix ref

d4b0a71

shorten line

76236e0

ickc and others added 9 commits August 26, 2021 15:59

add test_omp, test_tbb, test_workqueue

ccb4888

move import to the top

814398d

use assertRaises as suggested

92abe9d

Update numba/np/ufunc/parallel.py

7536815

Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

Update docs/source/user/threading-layer.rst

b11cf88

Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

Add wordings similar to that in "Setting the threading layer"

9199d93

to mention setting numba.config.THREADING_LAYER_PRIORITY

fix flake8

c19b36a

fix bug that stderr might include more than we print explicitly

7bf4c3e

fix flake8

a92b40e

ickc requested a review from stuartarchibald August 27, 2021 01:42

gmarkall 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 Aug 27, 2021

stuartarchibald reviewed Sep 10, 2021

View reviewed changes

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 Sep 10, 2021

ickc and others added 2 commits September 10, 2021 15:56

Update numba/np/ufunc/parallel.py

7afb27f

Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

Update docs/source/reference/envvars.rst

cbad639

Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

stuartarchibald approved these changes Oct 5, 2021

View reviewed changes

stuartarchibald mentioned this pull request Oct 5, 2021

Refactor unit tests scraping stdout/stderr in cases where alternatives are available. #7458

Closed

stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels Oct 5, 2021

stuartarchibald added this to the Numba 0.55 RC milestone Oct 5, 2021

sklam merged commit 52f74d8 into numba:master Oct 5, 2021

		print(' '.join(numba.config.THREADING_LAYER_PRIORITY))
		print("@%s@" % numba.threading_layer(), file=sys.stderr)

		@@ -306,6 +306,11 @@ def avx_default():
		DISABLE_JIT = _readenv("NUMBA_DISABLE_JIT", int, 0)

		# choose parallel backend to use

Conversation

ickc commented Jul 22, 2021

Reference an existing issue

Uh oh!

esc commented Jul 22, 2021

Uh oh!

esc commented Jul 22, 2021

Uh oh!

ickc commented Jul 22, 2021

Uh oh!

esc commented Jul 22, 2021

Uh oh!

esc commented Jul 22, 2021

Uh oh!

stuartarchibald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ickc commented Aug 27, 2021

Uh oh!

stuartarchibald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartarchibald Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

ickc Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

stuartarchibald Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

stuartarchibald Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

stuartarchibald Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

ickc Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

stuartarchibald Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

ickc commented Sep 10, 2021

Uh oh!

stuartarchibald left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants