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 get_model_status function (#1558) #1559

Merged
merged 17 commits into from
Aug 29, 2023

Conversation

sifisKoen
Copy link
Contributor

Resolve (#1558)

Add get_model_status issue.

Include the function plus the doc string with explanation of the function.

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Hey @sifisKoen, thanks for the PR! I made some comments to your changes. Most of thanks are just to remain consistent with the existing codebase.

It would be nice to have some tests added as well. You can take inspiration from TestHeadersAndCookies and define a TestModelStatus class with tests for the different use cases (loadable model, too big model, unknown model, model as url) and check the expected values. Please let me know if you have some questions!

src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
Comment on lines 102 to 105
loaded: bool
state: str
compute_type: str
framework: str
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to add a docstring here. This is important as it is used to generate our documentation pages. Here is an example of documented dataclass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got you 👍 I have already a doc string in the function that I implemented get_model_status I will do it to dataclass too.

src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
@sifisKoen
Copy link
Contributor Author

I will make these 8 changes.

sifisKoen and others added 6 commits July 20, 2023 01:53
Accepting the suggestion.

Co-authored-by: Lucain <[email protected]>
Accept the changes get_model_status function doc string.

Co-authored-by: Lucain <[email protected]>
Accept the string inclusion in Error raise.

Co-authored-by: Lucain <[email protected]>
Accept the two changes about the INFERENCE_ENDPOINT and get_session().

Co-authored-by: Lucain <[email protected]>
@sifisKoen
Copy link
Contributor Author

Hey,

I did the changes, as we discussed. Also I wrote the tests but I will need some time so to finish them. I will push the test in the next days. Sorry for the delay tho.

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Thanks for your work. I've added a few tiny comments but overall looks test.
Please let me know if you need some help for the tests!

Comment on lines 1320 to 1322
Identifier of the model for witch the status gonna be returned(retrieve). If model is not provided,
the model associated with this instance of InferenceClient will be used. The
identifier should not be a URL.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Identifier of the model for witch the status gonna be returned(retrieve). If model is not provided,
the model associated with this instance of InferenceClient will be used. The
identifier should not be a URL.
Identifier of the model for witch the status gonna be checked. If model is not provided,
the model associated with this instance of [`InferenceClient`] will be used. Only InferenceAPI service can be checked so the
identifier cannot be a URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello.

Thank you the you accepted my contribute. I will make the commits now, of your changes. 😄

I wrote some tests but I don't know if they are ok. Please let me know what I should to include and what I need to change.

class TestModelStatus(unittest.TestCase):

    # loadable model test case
    def test_loadable_model(self) -> None:
        client = InferenceClient()
        model_status = client.get_model_status("")
        self.assertEqual(model_status.loaded, False)
        self.assertEqual(model_status.state, "Loadable")
        self.assertEqual(model_status.compute_type, "gpu")
        self.assertEqual(model_status.framework, "text-generation-inference")
    
    # too big model test case
    def test_too_big_model(self) -> None:
        client = InferenceClient()
        model_status = client.get_model_status("")
        self.assertEqual(model_status.loaded, False)
        self.assertEqual(model_status.state, "TooBig")
        self.assertEqual(model_status.compute_type, "cpu")
        self.assertEqual(model_status.framework, "transformers")

    # loaded model test case
    def test_loaded_model(self) -> None:
        client = InferenceClient()
        model_status = client.get_model_status("")
        self.assertEqual(model_status.loaded, False)
        self.assertEqual(model_status.state, "Loaded")
        self.assertEqual(model_status.compute_type, "gpu")
        self.assertEqual(model_status.framework, "text-generation-inference")

    # unknown model test case
    def test_unknown_model(self) -> None:
        client = InferenceClient()
        with self.assertRaises(ValueError):
            client.get_model_status("unknown/model")

    # model as url test case    
    def test_model_as_url(self) -> None:
        client = InferenceClient()
        with self.assertRaises(NotImplementedError):
            client.get_model_status("https://unkown/model")

src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
src/huggingface_hub/inference/_common.py Outdated Show resolved Hide resolved
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jul 21, 2023

The documentation is not available anymore as the PR was closed or merged.

@Wauplin
Copy link
Contributor

Wauplin commented Jul 27, 2023

Hey @sifisKoen, yes that would be the idea for the tests! In details:

  • test_unknown_model and test_model_as_url are fine
  • test_loaded_model could check "bigcode/starcoder" (always loaded)
  • test_too_big_model could check "facebook/nllb-moe-54b" (always too big)
  • I would remove test_loadable_model as it's not easy to have a good example + it's not so important

Also for each test, I would merge those two lines but that's purely cosmetic:

        client = InferenceClient()
        model_status = client.get_model_status("")
        # Merge into =>
        # model_status = InferenceClient().get_model_status("")

@sifisKoen
Copy link
Contributor Author

@Wauplin hey dude I just upload the tests like we discussed 😄 . Please let me know if everything is or I need to change something. 🤓

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Hey @sifisKoen, thanks for completing the PR! I tested it locally and it works well! 🎉
I hope you don't mind but I've made some changes locally and pushed them to your branch, especially to support the same feature in AsyncInferenceClient. PR looks good to merge now! Thanks again 🤗

@Wauplin Wauplin merged commit 4c8e64e into huggingface:main Aug 29, 2023
11 of 14 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.

4 participants