MNT: updates to HF Hub, manage fairlearn deprecation by BenjaminBossan · Pull Request #392 · skops-dev/skops

BenjaminBossan · 2023-10-06T14:43:09Z

Some maintenance work to make skops future proof:

Ignore FutureWarning stemming from fairlearn: New pandas versions give FutureWarning for applymap. We have to wait for fairlearn to update this.
Make the widget attribute an array: Starting from HF Hub 0.18, it has to be an array.
InferenceApi is deprecated move to InferenceClient: API changed a little bit, had to be adjusted.

Note: DON'T MERGE THIS YET. The min version of HF Hub has been increased to 0.18 to test the changes but it actually should be lowered to 0.17 once CI is green for 0.18.

New pandas versions give FutureWarning for applymap. We have to wait for fairlearn to update this.

Starting from HF Hub 0.18, it has to be an array.

API changed a little bit, had to be adjusted.

This SHOULD NOT BE MERGED. Final min version will be 0.17 once the CI is green.

BenjaminBossan · 2023-10-06T14:57:46Z

@adrinjalali This should be ready to review. CI is partly red, but that just seems to be due to rate limit. I'll restart the tests later.

Edit: Seems to be a daily quota, so I guess this will have to wait a bit longer.

Edit: No longer red. Ready for review. If accepted, we just need to lower the hf hub requirement and then it's ready to be merged.

adrinjalali

Otherwise LGTM.

adrinjalali · 2023-10-09T10:21:46Z

-    )
+    client = InferenceClient(token=token)
+    res_bytes = client.post(json={"inputs": inputs}, model=repo_id)
+    res = json.loads(res_bytes.decode("utf-8"))


don't we have a test for this?

Yes, I forgot that we require [CI inference] in the commit message to trigger it. This is done now but of course we run into the rate limit again...

Should we remove the functionality then since the rate limit is so low?

It's mostly an issue for our CI, right? Which is probably why we made the test opt-in in the first place. I'd say we leave it, in case some users actually use the feature. If we want to remove it later on, we can do it properly with a deprecation cycle.

We can also add a random sleep between 1 and 100 hours to avoid hitting the limit ;-P

Hmm, not happy with a functionality that even our CI hits the limits pretty much every time. But we can merge and figure that out in a separate PR.

adrinjalali · 2023-10-09T13:21:36Z

    "scikit-learn": ("0.24", "install", None),
    "scikit-learn-intelex": ("2021.7.1", "docs", None),
-    "huggingface_hub": ("0.10.1", "install", None),
+    "huggingface_hub": ("0.18.0.rc0", "install", None),


should this change before merge?

Yes, the actual min version is 0.17. I can change it now or we can first ensure that all tests pass for 0.18 too -- which may take some time ;-)

Ok, let me know when they pass and then we can merge.

Okay, I'll let you know in a month or two.

Is it really that bad? So it's not something we can maintain. Let's fix the version merge here, then we deal with the deprecation

Yeah, the error now says that the daily quota has been exceeded. I don't remember it being this tight, not sure if it was reduced. We may be able to avoid it by using a HF employee token, but then we have the old issue of hiding it while still allowing it to be used in PRs, right?

We could perhaps also restrict the inference test to only run in 1 or 2 settings, instead of the full matrix, to avoid the rate limit.

At the end of the day, it's not a free service, and it doesn't match the vibe of this library anymore I would say. We added it because everybody could simply use it, but with the new rate limits, I don't see why we would keep supporting it.

I agree, the rate limit is pretty aggressive (still couldn't make all tests pass), so for practical purposes, it's not a free service anymore. I think we have a few options, I'll leave the decision up to you:

Still merge this PR, as I'm pretty sure it works (after adjusting the min version of course). Then create a new PR to deprecate the feature.

Don't merge this PR. Deprecate the feature.

Remove without deprecation.

I'd be happy to merge this PR, then I'll open another PR to deprecate the feature.

It was 0.18.0.rc0 for testing only.

jot-s-bindra · 2023-10-12T05:03:18Z

Its not related to this commit
I just wanted your help in setting up environment in local machine

git clone git@github.com:jot-s-bindra/skops.git
git init
git checkout -b new-feature
conda create -c conda-forge -n skops python=3.10
conda activate skops
python -m pip install -e ".[tests,docs]"
conda install -c conda-forge pre-commit
pre-commit install
pytest

during pytest i am recieving variable declare errors , modules not installed errors like
ERROR examples/plot_california_housing.py - KeyError: 'HF_HUB_TOKEN'
ERROR examples/plot_hf_hub.py - KeyError: 'HF_HUB_TOKEN'
ERROR examples/plot_intelex.py - KeyError: 'HF_HUB_TOKEN'
ERROR scripts/clean_skops.py - OSError: pytest: reading from stdin while output is captured! Consider using -s.
ERROR spaces/skops_model_card_creator/app.py - ModuleNotFoundError: No module named 'streamlit'
ERROR spaces/skops_model_card_creator/create.py - ModuleNotFoundError: No module named 'streamlit'
ERROR spaces/skops_model_card_creator/edit.py - ModuleNotFoundError: No module named 'streamlit'
ERROR spaces/skops_model_card_creator/gethelp.py - ModuleNotFoundError: No module named 'streamlit'
ERROR spaces/skops_model_card_creator/start.py - ModuleNotFoundError: No module named 'streamlit'
ERROR spaces/skops_model_card_creator/tasks.py - ModuleNotFoundError: No module named 'streamlit'

BenjaminBossan added 4 commits October 6, 2023 16:31

Ignore FutureWarning stemming from fairlearn

2233253

New pandas versions give FutureWarning for applymap. We have to wait for fairlearn to update this.

Make the widget attribute an array

af04046

Starting from HF Hub 0.18, it has to be an array.

InferenceApi is deprecated move to InferenceClient

d19871b

API changed a little bit, had to be adjusted.

For testing, increase min hfhub version to 0.18

f2a131a

This SHOULD NOT BE MERGED. Final min version will be 0.17 once the CI is green.

BenjaminBossan mentioned this pull request Oct 8, 2023

Model card widget : array format not provided by metadata_from_config #393

Closed

adrinjalali reviewed Oct 9, 2023

View reviewed changes

[CI inference] Empty commit with inference tests

e004671

adrinjalali reviewed Oct 9, 2023

View reviewed changes

Change min dependency of HF Hub to 0.17

e957c2c

It was 0.18.0.rc0 for testing only.

adrinjalali merged commit 933edbe into skops-dev:main Oct 11, 2023

Conversation

BenjaminBossan commented Oct 6, 2023

Uh oh!

BenjaminBossan commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jot-s-bindra commented Oct 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BenjaminBossan commented Oct 6, 2023 •

edited

Loading

BenjaminBossan Oct 9, 2023 •

edited

Loading