[sayakpaul]

Fixes #795

[sayakpaul]

I decided to keep the default to False because there are a couple of call sites which we are not testing here in this PR. But I would still prefer to keep to True as the default.

[danieldk]

I think we should avoid the same piling of argument that we had in the previous version of loading before the dependencies refactor. I think it's fine to have a check_arch argument on 'legacy' functions like get_kernel, etc. but it would be nice if we could keep get_kernel_with_resolver clean. So I propose to add a protocol Validator along the lines of:

class Validator(Protocol):
  def validate(self, *, tree: TreeNode[LocalKernel | RemoteKernel]):
    ...

No return type because a validator probably raises, then

class DependencyValidator:
  def validate(self, *, tree: TreeNode[LocalKernel | RemoteKernel]):
    validate_dependencies(...)

    for node in tree.deps.values():
      self.validate(node)

class ArchValidator:
  def validate(self, *, tree: TreeNode[LocalKernel | RemoteKernel]):
    _check_arch_incompatibility(tree.location.metadata, tree.location.variant_str)

    for node in tree.deps.values():
      self.validate(node)

@dataclass
class AllValidator:
    validators: list[Validator]

  def validate(self, *, tree: TreeNode[LocalKernel | RemoteKernel]):
    for validator in validators:
      validator(tree)

Then e.g. get_kernel could do something like:

return get_kernel_with_resolver(
  ...,
  validators=AllValidator(validator=[DependencyValidator(), ArchValidator] if check_arch else [DependencyValidator()])
)

Also dodges the need to have a default behavior (True or False) for get_kernel_with_resolver.

[sayakpaul]

I will debate that it has more layers of abstraction for what should be more explicit but I will table it for later and not block the PR.

[danieldk]

Small refinement after letting it sink in a bit. I think the recursion should be tree controlled, so we can have a method on DepTreeNode:

def validate(self, validator: Validator):
    for node in tree.deps.values():
      validator.validate(node)

And then the validator implementations can be simplified like this:

class ArchValidator:
  def validate(self, *, tree: TreeNode[LocalKernel | RemoteKernel]):
    _check_arch_incompatibility(tree.location.metadata, tree.location.variant_str)

[danieldk]

I will debate that it has more layers of abstraction for what should be more explicit but I will table it for later and not block the PR.

I think we end up collecting a lot of incidental arguments for functions and then they are hard to deprecate when we don't need them anymore or when we have to change the type of an argument. With the Resolver/Validator approach we don't have these issues, since it's composable and you can just not pass something anymore if it is not needed in the future.

People who like monoliths can still use get_kernel and friends.

[danieldk]

To add a bit to that, specifically to the present extension. check_arch is not an argument that we actually want to have. We should always check if the kernel is compatible with the compute hardware. We need check_arch as an argument because we need an escape hatch because the capability computation and representation is currently lacking here in some ways. If we solve the real issue, check_arch would become superfluous because it could always be True.

[sayakpaul]

Pushed the updates to change to the validator pattern.

Making the entrypoint to kernels over anything beyond get_kernel() is also not healthy, IMO. It doesn't always have to mean having to prefer monoliths; sometimes it's just about preference and going with the simplest alternative available.

[sayakpaul]

These properties are for convenience for better reporting of errors.

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[github-actions]

Coverage report — kernels/

Measured on: Python 3.10 / Torch 2.13.0.
Other CI configurations are not included in this number.
Hardware-gated code paths (ROCm/XPU/NPU/Darwin/Windows) are excluded or unreachable on the Linux+CUDA runner.

Total coverage: 86.7% — threshold: 80% — ✅

Per-file breakdown

Name	Stmts	Miss	Cover	Missing
src/kernels/__init__.py	14	0	100%
src/kernels/_system.py	6	1	83%	10
src/kernels/_versions.py	78	9	88%	47, 53-54, 57-58, 97, 119, 130, 136
src/kernels/archs.py	56	1	98%	95
src/kernels/backends.py	212	62	71%	40, 44, 48-51, 68, 90, 108, 117, 121, 125-127, 148, 157, 161, 165-167, 188, 199, 201, 208-211, 224, 228, 232-252, 260, 283-303
src/kernels/compat.py	8	1	88%	5
src/kernels/deps.py	81	1	99%	58
src/kernels/hf_hub.py	62	3	95%	18, 20, 114
src/kernels/importer.py	50	3	94%	110, 114, 117
src/kernels/install.py	21	7	67%	78-102
src/kernels/layer/__init__.py	6	0	100%
src/kernels/layer/_interval_tree.py	103	4	96%	23, 52, 147, 150
src/kernels/layer/device.py	48	14	71%	42, 47-49, 91, 96-98, 101, 149, 152, 155-157
src/kernels/layer/func.py	87	6	93%	91, 122, 194, 314, 340, 368
src/kernels/layer/globals.py	5	0	100%
src/kernels/layer/kernelize.py	74	8	89%	255, 281, 289-290, 296, 300, 316-318
src/kernels/layer/layer.py	216	15	93%	178, 223, 249, 367, 447-448, 460, 469, 477, 488, 517, 521, 534, 587, 617
src/kernels/layer/mode.py	14	0	100%
src/kernels/layer/repos.py	144	42	71%	27, 33, 36-43, 63-64, 70, 73-76, 90, 94, 103-104, 110, 113-116, 123-124, 130, 133-136, 143-144, 150, 153-156, 163-164, 170, 173-176, 257
src/kernels/load.py	69	2	97%	326, 365
src/kernels/locking.py	89	64	28%	35-83, 91-98, 102-125, 137, 152-159, 165-175, 179-186
src/kernels/python_deps.py	58	6	90%	59-60, 64-65, 101, 104
src/kernels/resolver.py	141	2	99%	182, 188
src/kernels/status.py	49	2	96%	23, 81
src/kernels/variants.py	278	19	93%	64, 95, 116, 146, 255-256, 298-301, 303, 387-394, 400-406, 437-443, 455-461
src/kernels/verify.py	88	1	99%	32
TOTAL	2057	273	87%

Updated by the Test kernels workflow on commit d2682c63e8ad9169136f5597ff182a75be43e434.

[danieldk]

Sorry, added a comment later (see the thread) that we probably should de recursion itself in DepTreeNode, since it's cleaner and validators don't have to worry how to traverse the tree. Only realized some time later, apologies!

[sayakpaul]

Yeah agreed. This way we can do tree.validate(ArchValidator()) and not have to do ArchValidator().validate(...).

[danieldk]

Super nice!