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

chore: refactor onedal interaction with backend and policies #2168

Open
wants to merge 61 commits into
base: main
Choose a base branch
from

Conversation

ahuber21
Copy link
Contributor

@ahuber21 ahuber21 commented Nov 14, 2024

Refined description

  • Remove policy from algorithm implementations.

    • Policy was used to control the device we're running on, but it's actually an implementation detail of the C++ backend.
    • This PR doesn't completely remove policy. But it puts it into a single place: The newly added BackendFunction class.
  • The device to run on is now controlled via the queue keyword. There is now a differentiation between user-facing functions (the API; e.g. model.fit(), model.predict(), ...), and internal methods (e.g. self._fit(), self.compute(), ...).

    • External functions have a default queue=None keyword argument, combined with a new decorator @supports_queue.
    • The @supports_queue decorator implements the logic that will determine the correct queue. If a queue is provided in the keyword argument, it will be used. Otherwise the queue will be determined from the config_context and the provided data, as configured via target_offload. This behavior is unchanged from the previous implementation. It was simply refactored.
    • Moreover, rather than having local policies and queues, there is now only a single global queue. It is managed in a SyclQueueManager namespace. The queue can be accessed from any internal method via SyclQueueManager.get_global_queue(). And in case local overloading of the queue is necessary, an execution context SyclQueueManager.manage_global_config(queue_or_None, data_or_None) is provided.
    • Internal functions shall no longer use a queue keyword argument. They should rather collect the global queue from SyclQueueManager (see above).
  • Rather than searching for module and function names with strings via self._get_backend(module_name, submodule_name, function_name, *args, **kwargs), we explicitly bind methods from the various backends using

    • @bind_default_backend(module_name): Use the available default backend (host or dpc).
    • @bind_spmd_backend(module_name): Use SPMD backend.
  • The decorators are used to decorate the existing function. For example

    @bind_default_backend("kmeans.clustering")
    def train(self, params, X_table, centroids_table): ...

    The module and submodule name is simply provided as "kmeans.clustering", as it would be in an import. This is not mandatory though and in other places a more generic def foo(self, *args, **kwargs) was used.
    It is useful but not mandatory to provide the correct function prototype with all arguments, as this will be picked up by linters and code completion tools.

  • In case of a name conflict, the decorator takes a keyword argument. For example

    def compute(self, *args, **kwargs):
      # This is an already existing function, so we cannot overload it with the decorator!
      # ...
      # more code
      ...
    
    @bind_default_backend("kmeans_init.init", lookup_name="compute")
    def backend_compute(self, params, X_table): ...

    The cleaner solution would obviously be to rename the symbols in the backend to something that doesn't conflict, like _compute. This can be done in a future PR.

A comment on the global queue: It is an imperfect attempt to reduce coupling between classes. Before refactoring we carefully crafted policies and queues in some classes (for example in the sklearnex module), so that the computation wouldn't crash after dispatching to onedal or even onedal.spmd. There was a strong coupling between the modules sklearnex->onedal->onedal.spmd. The situation improved in the sense that we are now able to perform some of the "temporary overloading of policies/queues" much closer to the part of the code that requires it. That is, rather than crafting a policy in sklearnex, we now just have a modification locally in onedal.spmd. This is much easier to understand and maintain.

In future, we should nevertheless try to eliminate the global queue variable, as it also can produce hard to debug issues, especially when the queue ends up in some kind of messed up state. The existing tests helped a lot to iron out many issues, but there's still room for ugly things to happen.

Original description

  • Refactor how and where methods are loaded from backend.
    • All functions are now explicitly declared as @abstractmethod and then populated using a decorator loaded from _backend.py.
    • It is now explicit where the functionality comes from.
    • We can start using the functions with self.infer(...) rather than the convoluted self._get_backend("X", "Y", "infer" , ...).
  • All dependencies from standard compute functions (compute(), infer(), ...) on backend are unified in _backend.py.
  • Backend wrapper class around the backend export in onedal/__init__.py to reduce the use of global variables.
  • More wrapper classes BackendManager, PolicyManager, BackendFunction that simplify interfacing with pybind/C++ backend functionality.
  • I removed unused imports because there were so many in the files I touched. I didn't stop at the files I modified and did the entire repo, which messed up the diff a little. Sorry. I am constantly rebasing so that all functionality is in a single commit and you can look at the diff of this single commit instead if you want to review.
    • Since this is becoming a quite complex PR, I reverted the unused imports changes. This will need its own PR.
  • Test update (WIP): Enables tests for iris and diabetes SVM algos on GPU. Updated dispatching seems to support GPU just fine. Feedback requested! @samir-nasibli
    • Confirmed missing implementation, I had a bug in my first version.

PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.

You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).

Checklist to comply with before moving PR from draft:

PR completeness and readability

  • I have reviewed my changes thoroughly before submitting this pull request.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes or created a separate PR with update and provided its number in the description, if necessary.
  • Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
  • I have added a respective label(s) to PR if I have a permission for that.
  • I have resolved any merge conflicts that might occur with the base branch.

Testing

  • I have run it locally and tested the changes extensively.
  • All CI jobs are green or I have provided justification why they aren't.
  • I have extended testing suite if new functionality was introduced in this PR.

Performance

  • I have measured performance for affected algorithms using scikit-learn_bench and provided at least summary table with measured data, if performance change is expected.
  • I have provided justification why performance has changed or why changes are not expected.
  • I have provided justification why quality metrics have changed or why changes are not expected.
  • I have extended benchmarking suite and provided corresponding scikit-learn_bench PR if new measurable functionality was introduced in this PR.

@ahuber21 ahuber21 force-pushed the dev/ahuber/refactor-onedal-backend-and-policies branch from 3e22a7e to 6e6b6ce Compare November 14, 2024 16:32
@icfaust
Copy link
Contributor

icfaust commented Nov 14, 2024

i guess this contrasts the approach i wanted to take in #2158 . what i had not liked about _get_backend was the indirection it was inducing in accessing the pybind11 generated package, and hoped to bring native syntax back. maybe worth further discussion?

@ahuber21
Copy link
Contributor Author

/intelci: run

@ahuber21 ahuber21 force-pushed the dev/ahuber/refactor-onedal-backend-and-policies branch from a3e1c8e to 0aa1669 Compare November 19, 2024 08:37
@ahuber21
Copy link
Contributor Author

/intelci: run

@ahuber21 ahuber21 marked this pull request as ready for review November 19, 2024 18:18
@ahuber21 ahuber21 force-pushed the dev/ahuber/refactor-onedal-backend-and-policies branch from 4c87853 to f0d2da7 Compare November 19, 2024 18:49
Copy link
Contributor

@ethanglaser ethanglaser left a comment

Choose a reason for hiding this comment

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

Initial quick comments, will look more in depth later

@@ -14,6 +14,6 @@
# limitations under the License.
# ==============================================================================

from .forest import RandomForestClassifier, RandomForestRegressor
from onedal.ensemble import RandomForestClassifier, RandomForestRegressor
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
from onedal.ensemble import RandomForestClassifier, RandomForestRegressor
from .ensemble import RandomForestClassifier, RandomForestRegressor

In general we have been using relative paths in cases like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My branch deletes forest.py and we're using the batch version directly. After removing EstimatorBaseSPMD there is no need to create child classes from the batch version. This change gets rid of a lot of code that doesn't really do anything and only makes it harder to understand what's happening.

https://github.com/intel/scikit-learn-intelex/blob/main/onedal/spmd/ensemble/forest.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevertheless I have added relative imports consistently (...ensemble in this case)

@ahuber21 ahuber21 force-pushed the dev/ahuber/refactor-onedal-backend-and-policies branch from f0d2da7 to 79a3108 Compare November 20, 2024 08:46
@ahuber21 ahuber21 marked this pull request as draft November 20, 2024 08:47
@ahuber21 ahuber21 force-pushed the dev/ahuber/refactor-onedal-backend-and-policies branch from 79a3108 to a820e94 Compare November 20, 2024 08:57
@ahuber21
Copy link
Contributor Author

/intelci: run

@ahuber21 ahuber21 marked this pull request as ready for review November 20, 2024 11:09
@Alexsandruss
Copy link
Contributor

Check PR Checklist / Close all checkboxes before moving from draft (pull_request)

@Alexsandruss Alexsandruss marked this pull request as draft November 20, 2024 11:17
@ahuber21
Copy link
Contributor Author

Performance and accuracy are not affected
image

@ahuber21 ahuber21 requested a review from ethanglaser November 21, 2024 07:56
@ahuber21
Copy link
Contributor Author

ahuber21 commented Nov 21, 2024

Reviewers, the performance regression in LogReg fit is reproducible and meaningful. I will investigate.

Performance degradation in LogReg is only compared to release (2025, 'P', 0), bot not main

image

--> It is not caused by this PR

@ahuber21 ahuber21 marked this pull request as ready for review November 21, 2024 10:55
@ahuber21 ahuber21 marked this pull request as draft November 21, 2024 10:56
@ahuber21
Copy link
Contributor Author

More checks required for SPMD changes, converting back to draft. But the non-SPMD contributions are still ready for review

@ahuber21
Copy link
Contributor Author

/intelci: run

1 similar comment
@ahuber21
Copy link
Contributor Author

/intelci: run

@ahuber21 ahuber21 force-pushed the dev/ahuber/refactor-onedal-backend-and-policies branch from 60cbc71 to 861ce67 Compare November 23, 2024 01:00
@ahuber21
Copy link
Contributor Author

/intelci: run

@ahuber21
Copy link
Contributor Author

/intelci: run

@ahuber21
Copy link
Contributor Author

/intelci: run

@ahuber21
Copy link
Contributor Author

/intelci: run

@ahuber21
Copy link
Contributor Author

/intelci: run

@ahuber21
Copy link
Contributor Author

/intelci: run

@ahuber21
Copy link
Contributor Author

/intelci: run

@ahuber21
Copy link
Contributor Author

/intelci: run

@ahuber21
Copy link
Contributor Author

Pre-commit only failed in a single spot due to bad luck with RNG. Not gonna re-run.

@ahuber21
Copy link
Contributor Author

/intelci: run

Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

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

I haven't dug into the nitty-gritty details yet, this was my first overview look, will provide more reviews in the next days.

@ahuber21
Copy link
Contributor Author

/intelci: run

@ahuber21
Copy link
Contributor Author

/intelci: run

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.

4 participants