[Misc] Add XET-Core Go binding for HuggingFace Hub Downloads#279
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @pallasathena92, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request integrates the xet-core Rust client into the Go ecosystem by introducing a CGO binding. The primary goal is to offer a high-performance solution for downloading data from the Hugging Face Hub. It includes a direct Go client for fine-grained control and a compatibility layer designed to seamlessly replace existing Hugging Face Hub Go client implementations, enhancing efficiency for data operations.
Highlights
- Go CGO Binding: Introduces a Go CGO binding to the Rust xet-core client, enabling Go applications to leverage its capabilities for efficient data operations.
- Hugging Face Hub Wrappers: Adds compatibility wrappers that mimic the Hugging Face Hub Go client API, simplifying migration and integration for existing users.
- Efficient Download Alternative: Provides a new, efficient alternative for Hugging Face Hub downloads, leveraging the performance and features of the xet-core Rust client.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces Go bindings for the Rust xet-core client, providing both a direct client and a compatibility layer for HuggingFace Hub downloads. The changes are comprehensive, including the CGO bindings, a compatibility layer, and an example application. The code is generally well-structured. My review focuses on improving error handling, testability, and code maintainability. I've identified a potential panic in the Rust FFI layer, unhandled errors in the Go code, and opportunities for refactoring to reduce code duplication.
| LogLevel: logLevel, | ||
| } | ||
|
|
||
| globalClient, _ = NewClient(config) |
There was a problem hiding this comment.
The error returned by xet.NewClient is ignored. If NewClient fails, globalClient will be nil. Subsequent calls to functions like HfHubDownload that rely on the global client will then fail with a less informative error message ("failed to create xet client"). It's safer to handle this potential error during initialization, for example by panicking, to signal a fatal setup problem.
| globalClient, _ = NewClient(config) | |
| var err error | |
| globalClient, err = NewClient(config) | |
| if err != nil { | |
| // Panicking in init is a common way to signal fatal initialization errors. | |
| panic(fmt.Sprintf("failed to initialize global xet client: %v", err)) | |
| } |
| if config.Token != "" || config.Endpoint != "" || config.CacheDir != "" { | ||
| // Create a custom client with the provided config | ||
| xetConfig := &Config{ | ||
| Endpoint: config.Endpoint, | ||
| Token: config.Token, | ||
| CacheDir: config.CacheDir, | ||
| MaxConcurrentDownloads: uint32(config.MaxWorkers), | ||
| EnableDedup: true, | ||
| } | ||
|
|
||
| if xetConfig.Endpoint == "" { | ||
| xetConfig.Endpoint = "https://huggingface.co" | ||
| } | ||
| if xetConfig.MaxConcurrentDownloads == 0 { | ||
| xetConfig.MaxConcurrentDownloads = 4 | ||
| } | ||
|
|
||
| var err error | ||
| client, err = NewClient(xetConfig) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| defer client.Close() | ||
| } |
There was a problem hiding this comment.
The logic for creating a custom xet.Client is duplicated in HfHubDownload (lines 87-110) and SnapshotDownload (lines 149-171). This duplication can make future maintenance more difficult and error-prone.
Consider refactoring this logic into a shared helper function to improve code reuse and maintainability. For example, a function like getClient(*DownloadConfig) could return the appropriate client (either the global one or a new temporary one) and a flag indicating if it needs to be closed by the caller.
| path, err := xet.SnapshotDownload(ctx, config) | ||
| if err != nil { | ||
| log.Fatalf("Snapshot download failed: %v", err) | ||
| } | ||
| fmt.Printf("Snapshot downloaded to: %s\n", path) | ||
| } else { | ||
| fmt.Printf("\nDownloading %s/%s using compatibility layer\n", repoID, filename) | ||
| path, err := xet.HfHubDownload(ctx, config) | ||
| if err != nil { | ||
| log.Fatalf("Download failed: %v", err) |
There was a problem hiding this comment.
The testCompatibilityLayer function directly calls xet.SnapshotDownload and xet.HfHubDownload instead of using the test hooks (snapshotDownloadHook and hfHubDownloadHook) defined at the top of the file. This is inconsistent with testDirectClient and makes this part of the example harder to unit test. For consistency and improved testability, please use the defined hooks.
| path, err := xet.SnapshotDownload(ctx, config) | |
| if err != nil { | |
| log.Fatalf("Snapshot download failed: %v", err) | |
| } | |
| fmt.Printf("Snapshot downloaded to: %s\n", path) | |
| } else { | |
| fmt.Printf("\nDownloading %s/%s using compatibility layer\n", repoID, filename) | |
| path, err := xet.HfHubDownload(ctx, config) | |
| if err != nil { | |
| log.Fatalf("Download failed: %v", err) | |
| path, err := snapshotDownloadHook(ctx, config) | |
| if err != nil { | |
| log.Fatalf("Snapshot download failed: %v", err) | |
| } | |
| fmt.Printf("Snapshot downloaded to: %s\n", path) | |
| } else { | |
| fmt.Printf("\nDownloading %s/%s using compatibility layer\n", repoID, filename) | |
| path, err := hfHubDownloadHook(ctx, config) | |
| if err != nil { | |
| log.Fatalf("Download failed: %v", err) | |
| } |
| return err | ||
| } | ||
| if !info.IsDir() { | ||
| relPath, _ := filepath.Rel(localDir, path) |
There was a problem hiding this comment.
The error returned by filepath.Rel is ignored. While it's unlikely to fail in this filepath.Walk context, it's a good practice to handle potential errors to make the code more robust. The WalkFunc can return an error to stop the walk, which is appropriate here.
relPath, err := filepath.Rel(localDir, path)
if err != nil {
return err
}
pallasathena92
force-pushed
the
yifeliu/xet
branch
from
0061edf to
6f15523
Compare
pallasathena92
force-pushed
the
yifeliu/xet
branch
from
6f15523 to
f496ee5
Compare
3 participants
/kind feature
What this PR does / why we need it:
Does this PR introduce a user-facing change?
None