Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Simplify utils/token.rs:get_token() #328

Merged

Conversation

polarathene
Copy link
Contributor

@polarathene polarathene commented May 18, 2024

Presently when a token is not considered valid via this utility method, it is treated as an empty string Some("") to hand over to the hf-hub crate API, but it should have instead been provided as None?

From the hf-hub crate, you can see that this would avoid adding a blank authorization HTTP header

    /// Sets the token to be used in the API
    pub fn with_token(mut self, token: Option<String>) -> Self {
        self.token = token;
        self
    }

    fn build_headers(&self) -> HeaderMap {
        let mut headers = HeaderMap::new();
        let user_agent = format!("unkown/None; {NAME}/{VERSION}; rust/unknown");
        headers.insert(USER_AGENT, user_agent);
        if let Some(token) = &self.token {
            headers.insert(AUTHORIZATION, format!("Bearer {token}"));
        }
        headers
    }

This PR change won't avoid this info log showing multiple times however:

INFO hf_hub: Token file not found "/root/.cache/huggingface/token"

Which is due to calling ApiBuilder::new() which initializes by checking a cache dir path for a token file (via cache.token())

Added context
    pub fn new() -> Self {
        let cache = Cache::default();
        Self::from_cache(cache)
    }

// ...

    pub fn from_cache(cache: Cache) -> Self {
        let token = cache.token();

Since mistral.rs is already setting the token via with_token() the upstream internal cache.token() seems redundant? But would presently require recreating from_cache() without the cache.token() line and providing a default Cache input:

// hf-hub should really have a separate default method similar to:
pub fn new_api_builder() -> ApiBuilder {
    Self {
        endpoint: "https://huggingface.co".to_string(),
        url_template: "{endpoint}/{repo_id}/resolve/{revision}/{filename}".to_string(),
        cache: Cache::default(),
        token: None,
        progress: true,
    }
}

// ...

let api = new_api_builder()
  .with_progress(true)
  .with_token(get_token(token_source)?)
  .build()?;

UPDATE: I've sent a PR upstream to address that, however it doesn't look like the crate is being maintained? Simple to review contributions already exist but have had no response.

Copy link

Code Metrics Report
  ===============================================================================
 Language            Files        Lines         Code     Comments       Blanks
===============================================================================
 Dockerfile              1           34           25            0            9
 Happy                   1          442          369            0           73
 JSON                    5            9            9            0            0
 Python                 21          741          622           21           98
 TOML                   15          390          352            1           37
-------------------------------------------------------------------------------
 Jupyter Notebooks       1            0            0            0            0
 |- Markdown             1           60           30           22            8
 |- Python               1           96           87            1            8
 (Total)                            156          117           23           16
-------------------------------------------------------------------------------
 Markdown               15         1026            0          758          268
 |- BASH                 6          205          192            0           13
 |- Python               6          121          110            0           11
 |- Rust                 3          185          172            9            4
 (Total)                           1537          474          767          296
-------------------------------------------------------------------------------
 Rust                   80        26617        24483          340         1794
 |- Markdown            38          382            0          377            5
 (Total)                          26999        24483          717         1799
===============================================================================
 Total                 140        29259        25860         1120         2279
===============================================================================
  

Copy link
Owner

@EricLBuehler EricLBuehler left a comment

Choose a reason for hiding this comment

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

This looks really great. When reviewing it, though, I had the idea to implement a fallback system similar to our ISQ fallback system. What do you think?

mistralrs-core/src/utils/tokens.rs Show resolved Hide resolved
@EricLBuehler EricLBuehler merged commit b65b3ab into EricLBuehler:master May 21, 2024
11 checks passed
@EricLBuehler
Copy link
Owner

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants