DEPR deprecate hub_utils.get_model_output#396
Conversation
BenjaminBossan
left a comment
BenjaminBossan
left a comment
There was a problem hiding this comment.
Thanks for adding this.
I'm not sure why test_push_download[pickle-False] is failing. At first I thought another rate limit, but I ran the tests a couple of times locally and they passed. Maybe it's a Windows issue.
| assert np.allclose(output, y_pred) | ||
|
|
||
|
|
||
| def test_get_model_output_deprecated(): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see, then it's good.
| "This feature is no longer free on hf.co and therefore this function will" | ||
| " be removed in the next release. Use `huggingface_hub.InferenceClient`" | ||
| " instead.", | ||
| FutureWarning, |
There was a problem hiding this comment.
I wonder if DeprecationWarning is more appropriate.
There was a problem hiding this comment.
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.
| # 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" |
There was a problem hiding this comment.
Technically speaking, it's still free, just heavily rate-limited.
There was a problem hiding this comment.
A philosophical question...
adrinjalali
left a comment
adrinjalali
left a comment
There was a problem hiding this comment.
I'm not sure why test_push_download[pickle-False] is failing. At first I thought another rate limit, but I ran the tests a couple of times locally and they passed. Maybe it's a Windows issue.
Can figure that out in another PR probably :)
| "This feature is no longer free on hf.co and therefore this function will" | ||
| " be removed in the next release. Use `huggingface_hub.InferenceClient`" | ||
| " instead.", | ||
| FutureWarning, |
There was a problem hiding this comment.
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.
| assert np.allclose(output, y_pred) | ||
|
|
||
|
|
||
| def test_get_model_output_deprecated(): |
There was a problem hiding this comment.
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.
| # 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" |
| # 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" |
There was a problem hiding this comment.
A philosophical question...
| "This feature is no longer free on hf.co and therefore this function will" | ||
| " be removed in the next release. Use `huggingface_hub.InferenceClient`" | ||
| " instead.", | ||
| FutureWarning, |
| assert np.allclose(output, y_pred) | ||
|
|
||
|
|
||
| def test_get_model_output_deprecated(): |
There was a problem hiding this comment.
I see, then it's good.
|
@BenjaminBossan so should we merge? |
2 participants
This deprecates the function, to be removed in the next released.