Skip to content
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.

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

Threads: add a one time warning to a no-arg nthreads() #48589

Closed

Conversation

IanButterworth
Copy link
Member

It seems helpful to communicate the (soft) deprecation of a no-arg nthreads() a bit more clearly.

re. #48580 (comment)

@warn """
In julia 1.9+ which supports threadpools (see [link to docs])
use of threads should be in the context of which threadpool is going to be used, so `nthreads()` should be
replaced with `nthreads(:default)`, or `nthreads(:interactive)` if interactive threads are requrested.""" maxlog = 1
Copy link
Member

Choose a reason for hiding this comment

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

There is not one simple replacement, as many usages must be replaced with task_local_storage instead or dropped entirely. But the replacement is almost never nthreads(:interactive)

Suggested change
replaced with `nthreads(:default)`, or `nthreads(:interactive)` if interactive threads are requrested.""" maxlog = 1
replaced with `nthreads(:default)` or `task_local_storage`, as appropriate.

Copy link
Member

@carstenbauer carstenbauer Feb 8, 2023

Choose a reason for hiding this comment

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

Also, AFAIU, the "number of threads" is - at least in some cases - a dynamic concept now and potentially/likely(?) even more so in the future. That is, the result of nthreads (with or without argument) isn't necessarily constant (per Julia session) anymore. This is something that users would need to be informed/warned about as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated with both suggestions

@IanButterworth
Copy link
Member Author

This will be quite noisy. Maybe that's a good thing?

from the doctests job, due to https://github.com/JuliaData/Parsers.jl/blob/78699fc450ca8c14ac073ddbec28d57cba31bb3c/src/Parsers.jl#L422

Precompiling project...
--
  | ✓ IOCapture
  | ✓ ANSIColoredPrinters
  | ✓ DocStringExtensions
  | ✓ Parsers
  | ✓ JSON
  | ✓ Documenter
  | 6 dependencies successfully precompiled in 11 seconds
  | 3 dependencies had warnings during precompilation:
  | ┌ Parsers [69de0a69-1ddd-5017-9359-2bf0b02dc9f0]
  | │  WARNING: method definition for dpeekbyte at /cache/build/default-amdci5-3/julialang/julia-master/doc/deps/packages/Parsers/34hDN/src/utils.jl:199 declares type variable T but does not use it.
  | └
  | ┌ JSON [682c06a0-de6a-54ab-a142-c8b1cf79cde6]
  | │  ┌ Warning: In julia 1.9+ which supports threadpools (and changing threadpool sizes during runtime),
  | │  │ use of threads should be in the context of which threadpool is going to be used, so `nthreads()` should be
  | │  │ replaced with `nthreads(:default)`, or `task_local_storage`, as appropriate.
  | │  └ @ Base.Threads deprecated.jl:335
  | └
  | ┌ Documenter [e30172f5-a6a5-5a46-863b-614d45cd2de4]
  | │  ┌ Warning: In julia 1.9+ which supports threadpools (and changing threadpool sizes during runtime),
  | │  │ use of threads should be in the context of which threadpool is going to be used, so `nthreads()` should be
  | │  │ replaced with `nthreads(:default)`, or `task_local_storage`, as appropriate.
  | │  └ @ Base.Threads deprecated.jl:335
  | └
  | ┌ Warning: In julia 1.9+ which supports threadpools (and changing threadpool sizes during runtime),
  | │ use of threads should be in the context of which threadpool is going to be used, so `nthreads()` should be
  | │ replaced with `nthreads(:default)`, or `task_local_storage`, as appropriate.
  | └ @ Base.Threads deprecated.jl:335

Also, it'd be nice if the warning warned about the nthreads() call site, not the @warn call site

@ Base.Threads deprecated.jl:335

@IanButterworth
Copy link
Member Author

IanButterworth commented Feb 8, 2023

What about only warn if an interactive threadpool is available?

@eval Threads function nthreads()
    if Threads.nthreads(:interactive) > 0
        @warn """
        An interactive threadpool is available, use of threads should be in the context of which threadpool is going to
        be used, so `nthreads()` should be replaced with `nthreads(:default)` or `nthreads(:interactive)`""" maxlog = 1
    end
    return threadpoolsize()
end

@JeffBezanson
Copy link
Member

This is silly. Just let people call nthreads! There is a default threadpool, so if you don't specify one you get the default, no problem. The right place to explain all this is in the manual.

@IanButterworth
Copy link
Member Author

ok. It's not really being deprecated then, right?

The manual uses nthreads() in various places, and even the Threadpools section does https://docs.julialang.org/en/v1.10-dev/manual/multi-threading/#man-threadpools

So it seems confusing to say it is (which is what prompted opening this PR)

@IanButterworth IanButterworth deleted the ib/nthreads_warn branch February 8, 2023 21:51
@simsurace
Copy link
Contributor

The original issue hasn't been addressed. I opened a new one: #48644

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs docs Documentation for this change is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants