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

Add judgment for whether model_id is a local path to speed up local model loading #523

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

chenwanqq
Copy link
Contributor

@chenwanqq chenwanqq commented Jul 2, 2024

In the original logic of marcos.os, regardless of whether model_id is a valid ID on Hugging Face, a request to Hugging Face is made, and if the request fails, it then attempts to load from the local environment. However, if the network environment cannot connect to Hugging Face normally, this step becomes very slow, with each API call attempt taking tens of seconds. Therefore, I have added a workaround here: first, it checks whether model_id is an absolute path. If it is, it directly loads from the local environment. This simple modification directly reduces the time spent on load_model_from_hf from 1 to 2 minutes to just a few microseconds 🤣
Edit: Now I change to check if model_id exists as a local path

@chenwanqq chenwanqq marked this pull request as ready for review July 2, 2024 08:45
Copy link

github-actions bot commented Jul 2, 2024

Code Metrics Report
  ===============================================================================
 Language            Files        Lines         Code     Comments       Blanks
===============================================================================
 Dockerfile              1           34           25            0            9
 Happy                   1          442          369            0           73
 JSON                   10           66           65            0            1
 Python                 36         1412         1220           38          154
 TOML                   18          495          446            2           47
-------------------------------------------------------------------------------
 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               21         1551            0         1171          380
 |- BASH                 5          101           98            0            3
 |- JSON                 1           12           12            0            0
 |- Python               5           98           88            0           10
 |- Rust                 4          221          198           10           13
 |- TOML                 2           75           63            0           12
 (Total)                           2058          459         1181          418
-------------------------------------------------------------------------------
 Rust                  131        44155        40029          779         3347
 |- Markdown            77          723           13          671           39
 (Total)                          44878        40042         1450         3386
===============================================================================
 Total                 220        48155        42154         1990         4011
===============================================================================
  

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.

Thank you for adding this! I think that if we check that the file exists, this may be more flexible. Is there a reason why you chose to use is_aboslute rather than exists?

@chenwanqq chenwanqq changed the title Add judgment for whether model_id is an absolute path to speed up local model loading Add judgment for whether model_id is a local path to speed up local model loading Jul 3, 2024
@chenwanqq
Copy link
Contributor Author

Thank you for adding this! I think that if we check that the file exists, this may be more flexible. Is there a reason why you chose to use is_aboslute rather than exists?

Hhh just not sure whether it would break some old logics. And If it is not, we can just check if model_id exists as a local path

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.

Thank you! We keep the logic for file path checking in these macros, so this should be fine.

@EricLBuehler EricLBuehler merged commit b0cfa1f into EricLBuehler:master Jul 3, 2024
10 checks passed
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