Conversation
|
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. |
updatesuname -a
# Darwin Mac 25.2.0 Darwin Kernel Version 25.2.0: Tue Nov 18 21:09:41 PST 2025; root:xnu-12377.61.12~1/RELEASE_ARM64_T6031 arm64
kernels init drbh/my-kernelnow clones the repo and removes the non platform specific dirs, and updates the output build toml file [general]
backends = ["metal"]
name = "my-kernel"
version = 1
[torch]
src = [
"torch-ext/torch_binding.cpp",
"torch-ext/torch_binding.h",
]
[kernel.my_kernel_metal]
backend = "metal"
depends = ["torch"]
src = [
"my_kernel_metal/my_kernel.mm",
"my_kernel_metal/my_kernel.metal",
]help message showskernels init --help |
kernels/src/kernels/init.py
Outdated
| backends = ["metal"] if sys.platform == "darwin" else ["cuda"] | ||
| else: | ||
| backends = [ | ||
| v.strip().lower() |
There was a problem hiding this comment.
Is stripping and lowercasing needed? I think all the whitespace should be consumed by the argument parser. I think it's better if people just have to give the arguments in lowercase.
There was a problem hiding this comment.
great suggestion the code is much cleaner in the latest commit, thanks!
kernels/src/kernels/cli.py
Outdated
| init_parser.add_argument( | ||
| "--backends", | ||
| nargs="+", | ||
| default=None, |
There was a problem hiding this comment.
I think we can use action="extend" and then the user can use something like --backends cuda rocm.
There was a problem hiding this comment.
ended up using "+" instead of extend so if a user specifies a backend it overrides the default, otherwise we default to metal on darwin and cuda in all other cases
kernels/src/kernels/init.py
Outdated
| backends = [ | ||
| v.strip().lower() | ||
| for item in args.backends | ||
| for v in item.split(",") |
There was a problem hiding this comment.
See above, I don't think we need extra parsing, we can let argparse handle it.
There was a problem hiding this comment.
same as above, updated in latest commit
kernels/src/kernels/cli.py
Outdated
| "--backends", | ||
| nargs="+", | ||
| default=None, | ||
| help="Backends to include ('all' or list like: cpu cuda metal rocm xpu). Defaults: cuda on Linux/Windows, metal on macOS.", |
There was a problem hiding this comment.
Since this is a finite list, it would be nice to use choices=.... We can also set the default to cuda or metal here. I think argparse will then even show it in the usage.
There was a problem hiding this comment.
same as above, updated in latest commit
kernels/src/kernels/init.py
Outdated
| def run_init(args: Namespace) -> None: | ||
| kernel_name = args.kernel_name | ||
| if args.backends is None: | ||
| backends = ["metal"] if sys.platform == "darwin" else ["cuda"] |
There was a problem hiding this comment.
same as above, updated in latest commit
kernels/src/kernels/init.py
Outdated
| sys.exit(1) | ||
| backends = [] | ||
| else: | ||
| valid = set(KNOWN_BACKENDS) |
There was a problem hiding this comment.
See above, best to let argparse do the work.
There was a problem hiding this comment.
fully agree, same as above, updated in latest commit
kernels/src/kernels/init.py
Outdated
| text = build_toml_path.read_text() | ||
| with open(build_toml_path, "rb") as f: | ||
| data = tomllib.load(f) | ||
| if "general" not in data: | ||
| return | ||
| kernel_table = data.get("kernel", {}) | ||
| if not isinstance(kernel_table, dict): | ||
| kernel_table = {} | ||
| remove_kernels = { | ||
| name | ||
| for name, cfg in kernel_table.items() | ||
| if isinstance(cfg, dict) and cfg.get("backend") not in set(backends) | ||
| } | ||
| backends_list = ", ".join(f'"{b}"' for b in backends) | ||
| new_line = f"backends = [{backends_list}]" | ||
| pattern = r"(\[general\][\s\S]*?)^\s*backends\s*=\s*\[[^\]]*\]" | ||
| new_text, count = re.subn(pattern, r"\1" + new_line, text, count=1, flags=re.M) | ||
| if remove_kernels: | ||
| new_text = _remove_kernel_sections(new_text, remove_kernels) | ||
| if count or remove_kernels: | ||
| build_toml_path.write_text(new_text) |
There was a problem hiding this comment.
I think it's cleaner to parse the TOML, modify the Python data structures and then serialize as TOML again. regexps are fragile.
There was a problem hiding this comment.
totally agree, the main reason I opt'ed for regex over toml is that I was concerned that writing toml back into the file would lose any comments/formatting added to the template.
I think its a reasonable trade off (more robust toml parsing, instead of comments) however my initial approach attempted to be comment preserving.
happy to update with the toml route, just wanted to flag the reasoning/tradeoff
There was a problem hiding this comment.
Maybe we should consider using tomlkit, since it can preserve comments?
There was a problem hiding this comment.
great suggestion, I've added tomlkit as a dep in the latest commits. Im still open to using the builtins if we don't see a benefit from preserving comments/whitespace, but the current changes are working well
kernels/src/kernels/utils.py
Outdated
| from kernels.metadata import Metadata | ||
|
|
||
| ENV_VARS_TRUE_VALUES = {"1", "ON", "YES", "TRUE"} | ||
| KNOWN_BACKENDS = ("cpu", "cuda", "metal", "rocm", "xpu", "npu") |
There was a problem hiding this comment.
agreed, updated in latest commit
334be5e to
658b0cc
Compare
60bc1f0 to
8796712
Compare
this PR adds a new
initsubcommand that clones a huggingfaceoutputs
now build
then run
outputs
and tested on a cuda device
outputs