Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions docs/source/builder/writing-kernels.md
Original file line number Diff line number Diff line change
Expand Up @@ -320,3 +320,14 @@ $ nix run .#ciTests.torch210-cxx11-cpu-x86_64-linux

When running the tests on a non-NixOS systems, make sure that
[the CUDA driver library can be found](https://danieldk.eu/Software/Nix/Nix-CUDA-on-non-NixOS-systems#solutions).

## Kernel docs

We provide a utility to generate a system card for a given kernel, utilizing
information from the `build.toml` and metadata. This system card provides a
reasonable starting point and is meant to be edited afterward by the kernel
developer.

To generate the card, use the `kernels create-and-upload-card ...` command.
Alternatively, specify `--create_card` while uploading the kernel builds
through `kernels upload`. Refer to the [documentation](../cli.md) to know more.
26 changes: 26 additions & 0 deletions kernels/src/kernels/cli/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,28 @@ def main():
default=None,
help="If set, the upload will be made to a particular branch of the provided `repo-id`.",
)
upload_parser.add_argument(
"--create-card",
action="store_true",
help="If set, a templated system card will be generated from the kernel.",
)
upload_parser.add_argument(
"--card-path",
type=str,
default=None,
help="Path to save the generated card to (only used with --create-card).",
)
upload_parser.add_argument(
"--description",
type=str,
default=None,
help="Description to introduce the kernel (only used with --create-card).",
)
upload_parser.add_argument(
"--create-pr",
action="store_true",
help="If set, it will create a PR on the repo-id (only used with --create-card).",
)
Comment on lines +112 to +116
Copy link
Member

Choose a reason for hiding this comment

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

I'd leave this out for the upload command. Having dependent options is not nice if not needed and it's strange that the kernel upload would not generate a PR, but the card would.

Comment on lines +95 to +116
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking more of just uploading the card, not creating one. E.g. if the user changed the card, it's nice that it gets uploaded as well along with the kernels. Presumably they already have created the card before. That means we have to settle on the name though, since I think on the Hub the card is always README.md and the README.md in a kernel Git repo may contain different information (more relevant to kernel developers/contributors).

So, maybe we should standardize on using CARD.md in the top-level directory of a kernel repo for the card and then kernel upload would upload it to the Hub as README.md. This is also in line with how the rest of how kernel building works - we go for very opinionated, there is only one way to do it, to enforce standardized, declarative source layouts.

I think orthogonal subcommands are the best, so ideally there would be a create-card subcommand that only creates a card. Then the upload subcommand uploads the card to the Kernel Hub along with the kernel. Maybe upload could have an --only-card option to only upload the card, but the default of kernels upload should do the right thing: upload the latest build variants and the latest card.

upload_parser.add_argument(
"--private",
action="store_true",
Expand Down Expand Up @@ -344,6 +366,10 @@ def upload_kernels(args):
branch=args.branch,
private=args.private,
)
if args.create_card:
if args.card_path is None:
args.card_path = str(Path(args.kernel_dir).resolve() / "README.md")
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but it would be nice to make all card paths Path and not str.

create_and_upload_card(args)


def create_and_upload_card(args):
Expand Down
22 changes: 22 additions & 0 deletions kernels/tests/test_kernel_upload.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import tempfile
from dataclasses import dataclass
from pathlib import Path
from unittest.mock import patch

import pytest

Expand All @@ -30,6 +31,10 @@ class UploadArgs:
repo_id: None
private: False
branch: None
create_card: bool = False
card_path: str = None
description: str = None
create_pr: bool = False


def next_filename(path: Path) -> Path:
Expand Down Expand Up @@ -120,3 +125,20 @@ def test_kernel_upload_deletes_as_expected():
str(filename_to_change) in k for k in repo_filenames
), f"{repo_filenames=}"
_get_hf_api().delete_repo(repo_id=REPO_ID)


@patch("kernels.cli.create_and_upload_card")
@patch("kernels.cli.upload_kernels_dir")
def test_kernel_upload_calls_create_and_upload_card(mock_upload_dir, mock_create_card):
with tempfile.TemporaryDirectory() as tmpdir:
args = UploadArgs(
kernel_dir=tmpdir,
repo_id=REPO_ID,
private=False,
branch=None,
create_card=True,
)
upload_kernels(args)

mock_create_card.assert_called_once_with(args)
assert args.card_path == str(Path(tmpdir).resolve() / "README.md")
Loading