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

[BUG] n_jobs validation is incorrect when it is negative #5990

Open
yarnabrina opened this issue Feb 23, 2024 · 3 comments
Open

[BUG] n_jobs validation is incorrect when it is negative #5990

yarnabrina opened this issue Feb 23, 2024 · 3 comments
Labels
bug Something isn't working module:plotting&utilities utilities including plottinng
Projects

Comments

@yarnabrina
Copy link
Collaborator

This is the current logic:

def check_n_jobs(n_jobs: int) -> int:
"""Check `n_jobs` parameter according to the scikit-learn convention.
Parameters
----------
n_jobs : int, positive or -1
The number of jobs for parallelization.
Returns
-------
n_jobs : int
Checked number of jobs.
"""
# scikit-learn convention
# https://scikit-learn.org/stable/glossary.html#term-n-jobs
if n_jobs is None:
return 1
elif not is_int(n_jobs):
raise ValueError(f"`n_jobs` must be None or an integer, but found: {n_jobs}")
elif n_jobs < 0:
return os.cpu_count() - n_jobs + 1
else:
return n_jobs

If n_jobs is negative, are we not requesting more CPU than that is available in L145-146?

I think we may use this, probably always:

https://github.com/joblib/joblib/blob/6310841f66352bbf958cc190a973adcca611f4c7/joblib/test/test_parallel.py#L611-L614

@yarnabrina yarnabrina added the bug Something isn't working label Feb 23, 2024
@fkiraly
Copy link
Collaborator

fkiraly commented Feb 23, 2024

hm, I did not know this code existed.

I thought the validation would be done anyway by joblib again.

The new parallelization abstraction entry point sktime.utils.parallel does not use it, btw.

Should we just get rid of it and let joblib handle it?

@yarnabrina
Copy link
Collaborator Author

yarnabrina commented Feb 23, 2024

I found this in #5989 who was modifying this function for n_jobs=-1 case, so I don't know if this is being used or not. If unused, removal makes sense.


It's being used I think.

image

@fkiraly
Copy link
Collaborator

fkiraly commented Feb 23, 2024

I know (well, since 5 min ago 😁 ), I found the same. Seems mostly used from:

  • transformers in the panel submodule
  • upon cursory inspection, in repeated boilerplate that could/should be replaced by base class broadcasting and parallelization

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working module:plotting&utilities utilities including plottinng
Projects
Bugfixing
Needs triage & validation
Development

No branches or pull requests

2 participants