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.

Sign up for GitHub

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

DEPR deprecate hub_utils.get_model_output #396

Merged
merged 3 commits into from
Oct 12, 2023
Merged

DEPR deprecate hub_utils.get_model_output #396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c850e19
DEPR deprecate hub_utils.get_model_output
adrinjalali Oct 12, 2023
593242e
add test
adrinjalali Oct 12, 2023
476893c
use new black
adrinjalali Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/changes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ v0.9
estimators. :pr:`384` by :user:`Reid Johnson <reidjohnson>`.
- Fix an issue with visualizing Skops files for `scikit-learn` tree estimators.
:pr:`386` by :user:`Reid Johnson <reidjohnson>`.
- :func:`skops.hug_utils.get_model_output` is deprecated and will be removed in version
0.10. :pr:`396` by `Adrin Jalali`_.

v0.8
----
Expand Down
14 changes: 12 additions & 2 deletions skops/hub_utils/_hf_hub.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import json
import os
import shutil
import warnings
from pathlib import Path
from typing import Any, List, Literal, MutableMapping, Optional, Sequence, Union

Expand Down Expand Up @@ -702,11 +703,16 @@ def download(
shutil.rmtree(path=cached_folder)


# TODO(v0.10): remove this function
def get_model_output(repo_id: str, data: Any, token: Optional[str] = None) -> Any:
"""Returns the output of the model using Hugging Face Hub's inference API.

See the :ref:`User Guide <hf_hub_inference>` for more details.

.. deprecated:: 0.9
Will be removed in version 0.10. Use ``huggingface_hub.InferenceClient``
instead.

Parameters
----------
repo_id: str
Expand Down Expand Up @@ -737,8 +743,12 @@ def get_model_output(repo_id: str, data: Any, token: Optional[str] = None) -> An
Also note that if the model repo is private, the inference API would not be
available.
"""
# TODO: the "type: ignore" should eventually become unncessary when hf_hub
# is updated
warnings.warn(
"This feature is no longer free on hf.co and therefore this function will"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically speaking, it's still free, just heavily rate-limited.

Copy link
Member Author

Choose a reason for hiding this comment

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

So it's not free 😁

Copy link
Collaborator

Choose a reason for hiding this comment

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

A philosophical question...

" be removed in the next release. Use `huggingface_hub.InferenceClient`"
" instead.",
FutureWarning,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if DeprecationWarning is more appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

DeprecationWarning is not shown by default, but FutureWarning is, that's why in sklearn we switched to FutureWarning to make sure people actually see it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL. Makes sense.

)
model_info = HfApi().model_info(repo_id=repo_id, use_auth_token=token) # type: ignore
if not model_info.pipeline_tag:
raise ValueError(
Expand Down
9 changes: 8 additions & 1 deletion skops/hub_utils/tests/test_hf_hub.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,8 @@ def test_inference(

X_test = data.data.head(5)
y_pred = model.predict(X_test)
output = get_model_output(repo_id, data=X_test, token=HF_HUB_TOKEN)
with pytest.warns(FutureWarning):
output = get_model_output(repo_id, data=X_test, token=HF_HUB_TOKEN)

# cleanup
client.delete_repo(repo_id=repo_id, token=HF_HUB_TOKEN)
Expand All @@ -512,6 +513,12 @@ def test_inference(
assert np.allclose(output, y_pred)


def test_get_model_output_deprecated():
Copy link
Collaborator

Choose a reason for hiding this comment

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

If, in the test above, you add the match to pytest.warns, this test would be completely redundant, right? This might be a worthwhile change, so that we avoid hitting the endpoint even more often. If you want to leave this as a separate test, it should have the @pytest.mark.network and @pytest.mark.inference decorators markers, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here I'm not passing any token and not passing a valid model name, so the function would fail anyway (hence the pytest.raises(Exception), therefore I don't think it even needs a network marker, and it can be in all situations and not only with a specific commit message or a merker for pytest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, then it's good.

with pytest.raises(Exception):
with pytest.warns(FutureWarning, match="This feature is no longer free"):
get_model_output("dummy", data=iris.data)


def test_get_config(repo_path, config_json):
config_path, file_format = config_json
config = get_config(repo_path)
Expand Down