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

FEA Support custom lib controllers #138

Merged
merged 16 commits into from Jul 11, 2023

Conversation

jeremiedbb
Copy link
Collaborator

@jeremiedbb jeremiedbb commented Jul 5, 2023

Fixes #137

The goal is to expose the LibController abstract class, and custom controllers should inherit from it. This PR also exposes a new function register that simply adds the custom controller to the list of controllers to try.

TODO:

  • documentation

@jeremiedbb jeremiedbb changed the title FEA Support custom lib controllers [WIP] FEA Support custom lib controllers Jul 5, 2023
Copy link
Collaborator

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

A few comment on the doc, but the implem is super clean!!
I like this very much :)

Thanks @jeremiedbb

threadpoolctl.py Show resolved Hide resolved
threadpoolctl.py Outdated Show resolved Hide resolved
threadpoolctl.py Outdated Show resolved Hide resolved
threadpoolctl.py Show resolved Hide resolved
threadpoolctl.py Show resolved Hide resolved
threadpoolctl.py Show resolved Hide resolved
Copy link
Contributor

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Once the doc is complete, and assuming the CI is fixed (ModuleNotFoundError: No module named 'mesonpy', apparently a new build time dependency for the BLIS CI entries) +1 for merge.

I like the new design, it's much cleaner in addition to making it possible to register a third party lib handler.

@jeremiedbb
Copy link
Collaborator Author

jeremiedbb commented Jul 6, 2023

Once the doc is complete, and assuming the CI is fixed (ModuleNotFoundError: No module named 'mesonpy', apparently a new build time dependency for the BLIS CI entries) +1 for merge.

this error is because I tried to set --no-build-isolation. I just reverted it. You'll see the original error. I've not been able to understand what's wrong with this install so far... (the CI broke ~2 weeks ago)

@ogrisel
Copy link
Contributor

ogrisel commented Jul 7, 2023

Have you tried to mamba / pip install ninja? It might be a new runtime dependency of numpy when building in editable mode.

@jeremiedbb jeremiedbb changed the title [WIP] FEA Support custom lib controllers FEA Support custom lib controllers Jul 7, 2023
@jeremiedbb
Copy link
Collaborator Author

I added all the doc I wanted to add. This is ready for another round of reviews :)

tests/_pyMylib/__init__.py Show resolved Hide resolved
tests/_pyMylib/__init__.py Outdated Show resolved Hide resolved
tests/_pyMylib/__init__.py Outdated Show resolved Hide resolved
tests/_pyMylib/__init__.py Show resolved Hide resolved
tests/_pyMylib/__init__.py Show resolved Hide resolved
Copy link
Contributor

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

+1 as well once @tomMoral inline comments suggestion to make the sample plugin more educational are included.

Copy link
Collaborator

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

LGTM, great feature @jeremiedbb

Copy link
Contributor

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

A few more improvement. @jeremiedbb feel free to merge if you agree with them.

threadpoolctl.py Outdated Show resolved Hide resolved
threadpoolctl.py Outdated Show resolved Hide resolved
identify the library. e.g. "libopenblas" for libopenblas.so.

and implement the following methods: `get_num_threads`, `set_num_threads` and
`get_version`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`get_version`.
`get_version`.
A library controller can optionally specify the `check_symbols` attribute valued
with a tuple of `str` in cases where the `filename_prefixes` attribute is not
specific enough to unambiguously identify a dynamically linked native library.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not True yet. We only check the symbols of libblas on windows to workaround conda renaming. We could try to implement support for that but I'd rather do that in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep that for a follow-up PR then.

the filename of each library with the `filename_prefixes`. If a match is found, a
controller is instantiated and a handler to the library is stored in the `dynlib`
attribute as a `ctypes.CDLL` object. It can be used to access the necessary symbols
of the shared library to implement the above methods.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
of the shared library to implement the above methods.
of the shared library to implement the above methods assuming the optional
preconditions defined in the `check_symbols` attribute are met.

jeremiedbb and others added 2 commits July 11, 2023 11:24
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@ogrisel ogrisel merged commit c2f4b0a into joblib:master Jul 11, 2023
20 checks passed
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jul 17, 2023
3.2.0 (2023-07-13)
==================

- Dropped support for Python 3.6 and 3.7.

- Added support for custom library controllers. Custom controllers must inherit from
  the `threadpoolctl.LibController` class and be registered to threadpoolctl using the
  `threadpoolctl.register` function.
  joblib/threadpoolctl#138

- A warning is raised on macOS when threadpoolctl finds both Intel OpenMP and LLVM
  OpenMP runtimes loaded simultaneously by the same Python program. See details and
  workarounds at https://github.com/joblib/threadpoolctl/blob/master/multiple_openmp.md.
  joblib/threadpoolctl#142
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.

Support registering user-defined LibController
3 participants