-
Notifications
You must be signed in to change notification settings - Fork 8
feat: compliant cli checker for correct variants and dependencies #121
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: main
Are you sure you want to change the base?
Conversation
bcc946d
to
bd4f49f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice to have this! Added a bunch of comments.
compliant/Cargo.toml
Outdated
@@ -0,0 +1,29 @@ | |||
[package] | |||
name = "compliant" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should name it kernel-compliance-check
? A bit more descriptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, updated dir and tool name in latest changes. Thanks!
compliant/src/lib.rs
Outdated
use anyhow::{Context, Result}; | ||
use clap::{Parser, Subcommand, ValueEnum}; | ||
use colored::Colorize; | ||
use hf_hub::api::tokio::{ApiBuilder, ApiError}; | ||
use hf_hub::{Repo, RepoType}; | ||
use kernel_abi_check::{check_manylinux, check_python_abi, Version}; | ||
use object::Object; | ||
use once_cell::sync::Lazy; | ||
use serde::{Deserialize, Serialize}; | ||
use std::fmt; | ||
use std::fs; | ||
use std::path::{Path, PathBuf}; | ||
use thiserror::Error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use anyhow::{Context, Result}; | |
use clap::{Parser, Subcommand, ValueEnum}; | |
use colored::Colorize; | |
use hf_hub::api::tokio::{ApiBuilder, ApiError}; | |
use hf_hub::{Repo, RepoType}; | |
use kernel_abi_check::{check_manylinux, check_python_abi, Version}; | |
use object::Object; | |
use once_cell::sync::Lazy; | |
use serde::{Deserialize, Serialize}; | |
use std::fmt; | |
use std::fs; | |
use std::path::{Path, PathBuf}; | |
use thiserror::Error; | |
use std::fmt; | |
use std::fs; | |
use std::path::{Path, PathBuf}; | |
use anyhow::{Context, Result}; | |
use clap::{Parser, Subcommand, ValueEnum}; | |
use colored::Colorize; | |
use hf_hub::api::tokio::{ApiBuilder, ApiError}; | |
use hf_hub::{Repo, RepoType}; | |
use kernel_abi_check::{check_manylinux, check_python_abi, Version}; | |
use object::Object; | |
use once_cell::sync::Lazy; | |
use serde::{Deserialize, Serialize}; | |
use thiserror::Error; |
I like this separation of imports between stdlib <-> external crates <-> local.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yea thats better, updated in latest changes. Thanks
compliant/src/lib.rs
Outdated
/// Automatically fetch repositories if not found locally | ||
#[arg(short, long, default_value = "true")] | ||
auto_fetch: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be nice if auto-fetching is the default:
- If cached and up-to-date, immediately use local.
- Otherwise download and use local.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed and improved to always check if the local source is the latest hash of that revision and added and sync if needed. Also added --force
to enable fetching even if you have the copy locally
compliant/src/lib.rs
Outdated
#[derive(Subcommand)] | ||
pub enum Commands { | ||
/// List fetched repositories with build variants | ||
List { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? I like that with normal Hub/kernels
usage there is not a real distinction between local/remote. You give a repo name and the library takes care of everything. With list
and non-auto download (see below), you have to think about where things live, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea that makes sense, removed in latest commits
compliant/src/lib.rs
Outdated
} | ||
|
||
async fn fetch_compliant_variants() -> Result<(Vec<String>, Vec<String>)> { | ||
let url = "https://raw.githubusercontent.com/huggingface/kernel-builder/refs/heads/main/build-variants.json"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe should move this build-variants.json
into the src
directory of the checker and bake it into the binary? I think this allows more control over future changes of the JSON file and avoids breaking all past versions if the format changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes alot of sense. For now i've added a new build.rs
script that vendors the json in a rs file at build time so its baked into the binary. I think this should add alot more stability and avoids fetching the variants during each run
compliant/src/lib.rs
Outdated
let hash = content.trim(); | ||
let snapshot_dir = repo_path.join(format!("snapshots/{}", hash)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can directly get the correct repo directory from hf-hub, so that we don't have to care about the on-disk representation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, this has been simplified to prefer the hf-hub in latest commits
compliant/src/lib.rs
Outdated
let repo_path = get_repo_path(repo_id, cache_dir); | ||
|
||
// Check if repository exists locally | ||
if !repo_path.exists() || !repo_path.join("refs/main").exists() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fails on other branches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now relying on hf-hub
compliant/src/lib.rs
Outdated
} | ||
|
||
// Re-check after potential fetch | ||
let ref_file = repo_path.join("refs/main"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fails on other branches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now relying on hf-hub
compliant/src/lib.rs
Outdated
.with_context(|| format!("Failed to read ref file: {:?}", ref_file))?; | ||
|
||
let hash = content.trim(); | ||
let snapshot_dir = repo_path.join(format!("snapshots/{}", hash)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get the directory from hf-hub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now relying on hf-hub
compliant/src/main.rs
Outdated
@@ -0,0 +1,155 @@ | |||
use anyhow::{Context, Result}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit: we use eyre
in build2cmake
and kernel-abi-check
. Maybe we should use the same here for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, i've updated to prefer eyre over anyhow in the latest changes. Thanks!
This PR adds a new cli tool
compliant
that checks kernels for compliance.check args
Usage
list all kernels (in cache and have a build variant)
$ compliant list . ├── kernels-community/activation ├── kernels-community/deformable-detr ├── kernels-community/flash-mla ├── kernels-community/quantization ╰── 4 kernel repositories found
checking a repo
with
--long
output for variant specific abi compatibilityscreenshot to show coloring
Checks
build
dir