Skip to content

Conversation

@ngoldbaum
Copy link
Collaborator

Fixes #145.

@nickodell
Copy link
Contributor

Testing notes:

  1. This works in allowing me to add a limit to the number of threads in a test. I tested this using the example from the associated issue.
  2. I get a strange warning when using this decorator.

Code:

    @pytest.mark.parallel_threads_limit(4)  # 0.4 GiB per thread RAM usage
    @pytest.mark.fail_slow(5.0)
    def test_workers(self):

Test invocation:

spin test -s optimize -v -- -s -k TestLM

Result:

scipy/optimize/tests/test_least_squares.py::TestLM::test_workers here
PASSED [thread-unsafe]: test is marked as single-threaded

Of course, the test is not marked as single-threaded. The test is running in only one thread, but that is the result of the command line --parallel-threads option not being passed.

@ngoldbaum
Copy link
Collaborator Author

ngoldbaum commented Nov 18, 2025

Thanks! I tweaked things a little so now that message will only trigger if you limit a test to one thread.

Copy link
Member

@lysnikolaou lysnikolaou left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @ngoldbaum!

Should we open an issue to deprecate the parallel_threads marker?

@ngoldbaum
Copy link
Collaborator Author

Sure, go ahead. I can probably send a PR in later so we can cut a release for SciPy's benefit.

@nickodell
Copy link
Contributor

Sure, go ahead. I can probably send a PR in later so we can cut a release for SciPy's benefit.

There's no rush on our end.

We're not blocked on this, and we have a workaround, which is to set parallel_threads(1) on the offending test. I just thought it would be useful to better match the intent people have when adding this decorator for performance reasons.

Two other things I was thinking about on this PR:

  1. If I understand correctly, the precedence when both decorators are used is parallel_threads > parallel_threads_limit ? Is that the best order? On one hand, it makes sense that the decorator which sets the thread count wins, as it is more specific, but on the other hand, I'm not sure it matches your intentions around deprecation.
  2. It's a little inconsistent that one can use "auto" inside parallel_threads() but not parallel_threads_limit(). It might be good to refactor that parsing logic into a new function.

@ngoldbaum
Copy link
Collaborator Author

I'm not sure it matches your intentions around deprecation.

I wanted to avoid breaking any existing users of the parallel_threads decorator, so I left it alone. Only new code will use the parallel_threads_limit decorator. Existing or new tests that use the parallel_threads decorator will trigger a deprecation warning.

I considered banning using both decorators but thought a deprecation would handle that case without any more logic here.

It might be good to refactor that parsing logic into a new function.

Do you have a usecase for @parallel_threads_limit("auto")? IMO that wouldn't really do anything useful - I wouldn't want to have a limit that varies from computer to computer.

@nickodell
Copy link
Contributor

Do you have a usecase for @parallel_threads_limit("auto")? IMO that wouldn't really do anything useful - I wouldn't want to have a limit that varies from computer to computer.

I agree. I mostly suggested it for API consistency. Imagine someone is doing a mass find-replace around their codebase of parallel_threads() to parallel_threads_limit(). Would be useful if those two did the same thing.

@ngoldbaum
Copy link
Collaborator Author

Would be useful if those two did the same thing.

Fair enough.

@ngoldbaum ngoldbaum force-pushed the thread-limit-decorator branch from 3cdb77a to c1e0902 Compare November 18, 2025 18:59
@ngoldbaum
Copy link
Collaborator Author

Alright, let's merge this.

@ngoldbaum ngoldbaum merged commit 3b618c3 into Quansight-Labs:main Nov 19, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature request: decorator that sets upper limit on thread count

3 participants