Skip to content

Conversation

@mcabbott
Copy link
Contributor

@mcabbott mcabbott commented Jul 1, 2020

Ref JuliaLang/julia#36360

Note that I didn't export this, leaving it as Compat.get_num_threads(), but did import the version from LinearAlgebra when defined.

Also, I copied the new version of set_num_threads() here, rather than calling the existing one.

@martinholters
Copy link
Member

Should set_num_threads() also be mentioned in the README and set_num_threads(nothing) be tested (that it doesn't error, at least)? Or why is it included here?

@mcabbott
Copy link
Contributor Author

mcabbott commented Jul 2, 2020

I tried to minimally adapt the PR's tests, but I agree that it's odd that set_num_threads(::Nothing) isn't tested, I can add something. The difference from the existing function is that it accepts nothing, and that it prints more warnings. Seemed simpler to copy the whole new implementation than to split it.

Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

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

LGTM.

Compat.set_num_threads(default)

if VERSION < v"1.6.0-DEV.322"
# These tests from PR rely on internal functions which would be BLAS. not Compat.
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit ugly but I can't think of a better way either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could also dump these & call testing the overall behaviour enough, here.

Copy link
Member

Choose a reason for hiding this comment

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

But IIUC, some of the code below explicitly invokes code paths not hit by usual CI, so might be worth it. Not sure.

Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

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

Oh, that approval was a bit too fast:

WARNING: Compat.Sys is deprecated, use Base.Sys instead.

  likely near /home/travis/build/JuliaLang/Compat.jl/test/runtests.jl:468

Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

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

Will merge once CI passes (assuming it doesn't surprise me with new warnings).

@martinholters martinholters merged commit 04b2976 into JuliaLang:master Jul 2, 2020
@mcabbott mcabbott deleted the get_threads branch July 2, 2020 12:01
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.

2 participants