[RFC] Contrib test suite + tests for timm and sentence_transformers #1200

Wauplin · 2022-11-18T11:09:37Z

First discussed in #1190. Goal is to detect proactively breaking changes and deprecation warnings in downstream libraries. This is a very first implementation with tests for timm and sentence_transformers libraries to validate the concept. I think for a start we can have a github workflow only triggered
manually or contrib_ci_* branches. It doesn't really make sense to run it on each PR or on main.

How it works ?

The contrib folder contains simple end-to-end scripts to test integration of huggingface_hub in downstream libraries. The main goal is to proactively notice breaking changes and deprecation warnings. Each library is tested in its own virtualenv (with its own dependencies). Here is the workflow for "timm" library:

Create a virtualenv under ./contrib/timm/.venv
Install lib requirements ./contrib/timm/requirements.txt: install timm from main branch. Configurable for each lib.
Uninstall huggingface_hub (if any)
Install huggingface_hub from source code to test against latest version
Run timm tests pytest ./contrib/timm

See #1200 (comment) for more details.

How to add a new library ?

To add another contrib lib, one must:

Create a subfolder with the lib name. Example: ./contrib/transformers
Create a requirements.txt file specific to this lib. Example ./contrib/transformers/requirements.txt
Implements tests for this lib. Example: ./contrib/transformers/test_push_to_hub.py
Run make style to edit makefile and .github/workflows/contrib-tests.yml to add the new lib.

Run contrib tests in CI

Contrib tests can be manually triggered in GitHub with the Contrib tests workflow. CI is also triggered on branches starting by ci_contrib_*

Tests are not run in the default test suite (for each PR) as this would slow down development process. The goal is to notice breaking changes, not to avoid them. In particular, it is interesting to trigger it before a release to make sure it will not cause too much friction.

Run contrib tests locally

Tests are separated to avoid conflicts between version dependencies. Before running tests, a virtual env must be setup for each contrib library. To do so, run:

# Run setup in parallel to save time 
make contrib_setup -j4

# Run tests
# Optional: -j4 to run in parallel. Output will be messy in that case.
make contrib_test -j4

# Run only "timm" tests
make contrib_setup_timm
make contrib_test_timm

See #1200 (comment) for more details.

Todo:

HuggingFaceDocBuilderDev · 2022-11-18T11:18:58Z

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

osanseviero

Very cool! I left some minor questions, but overall this looks to be going in the right direction! 🔥

contrib/requirements.txt

.github/workflows/contrib-tests.yml

contrib/test_timm.py

contrib/requirements.txt

Wauplin · 2022-11-21T17:56:44Z

@osanseviero thanks for your feedback, it helped a lot !
I make some changes to the contrib structure. Now I have a common requirements.txt. For each lib, I have a folder with a test file and another requirements.txt. The workflow is now to launch 1 process/job per dependent library:

create virtualenv for timm
install common requirements
install timm requirements
uninstall huggingface_hub (if any)
install huggingface_hub from source code
run timm tests

This way we don't have to handle conflicts between different dependencies / versions. It makes local tests a bit more complex as it requires 1 env per contrib. Hopefully not much people will really need to run those stuff locally. What I plan to do is to add a script (either separate, either in makefile) to handle all the stuff with the venvs. WDYT in general ?

Wauplin · 2022-11-22T10:01:57Z

Refactored a bit the makefile. Now possible to setup and run all tests locally by running:

# Setup all virtualenvs
make contrib_setup

# Run all tests
make contrib_tests

# Setup and run all tests at once
make contrib

# Delete all virtual envs (if corrupted)
make contrib_clear

And for a specific lib:

# Setup timm tests
make contrib_setup_timm

# Run timm tests
make contrib_test_timm

# Setup and run timm tests at once
make contrib_timm

# Delete timm virtualenv
make contrib_clear_timm

osanseviero

Looking neat! I want to make a second pass through this PR and also let's see if others have any thoughts

contrib/README.md

Co-authored-by: Omar Sanseviero <osanseviero@gmail.com>

.github/workflows/contrib-tests.yml

osanseviero · 2022-11-25T10:30:58Z

.github/workflows/contrib-tests.yml

+  workflow_dispatch:
+  push:
+    branches:
+      - ci_contrib_*


should we run this for main as well?

I don't think so. If we want to make trigger it manually from main it's possible but if we run it all the time, the main branch could end up in a ❌ status as I expect contrib tests to fail (code can break in downstream library without a change on our side).

.github/workflows/contrib-tests.yml

contrib/timm/test_timm.py

osanseviero · 2022-11-25T11:08:45Z

contrib/utils.py

+
+
+@contextlib.contextmanager
+def production_endpoint() -> Generator:


Is this really needed? With HF_ENDPOINT you could change the endpoint you're using

Is this really needed?

I'd say yes, kinda the same as what already exists in the hfh tests/. The problem with HF_ENDPOINT is that it is evaluated only once at startup. What I want here is to make all calls to staging environment (especially pushing to repos) except some calls that have to be made to production environment (especially loading models).

Another solution could be to upload test models to staging but then we wouldn't notice if a model changed in production.

Wauplin · 2022-11-25T12:28:13Z

Thanks for the review @osanseviero. I made some changes and addresses all of your comments.
Let's have a final review from @LysandreJik once he is back from holidays and then merge the PR.

LysandreJik

Looks great! I left a few comments

LysandreJik · 2022-11-28T13:49:54Z

.github/workflows/contrib-tests.yml

+        contrib: [
+          "sentence_transformers",
+          "timm",
+        ]


Great idea to use a matrix here!

Should this matrix be governed by a ls first so that we have individual jobs for each folder under contrib and without the need to specify each folder here? (nitpick though, this should (or shouldn't) be done in a follow up PR)

Added in f20ab77 and 6cb96e7 a script that list contrib tests and update the Makefile and github workflow file accordingly. Script is integrated with make quality and make style to make it easy for contributors.

LysandreJik · 2022-11-28T13:52:43Z

contrib/README.md

+4. Edit `makefile` to add the lib to `CONTRIB_LIBS` variable. Example: `CONTRIB_LIBS := timm transformers`
+5. Edit `.github/workflows/contrib-tests.yml` to add the lib to `matrix.contrib` list. Example: `contrib: ["timm", "transformers"]`


I'd eventually look into automating these two so that it's slightly less error-prone, but as said above: nitpick

Now done by make style and make quality. Good call to reduce contribution efforts.

LysandreJik · 2022-11-28T13:53:18Z

contrib/README.md

+
+Contrib tests can be [manually triggered in GitHub](https://github.com/huggingface/huggingface_hub/actions) with the `Contrib tests` workflow.
+
+Tests are not run in the default test suite (for each PR) as this would slow down development process. The goal is to notice breaking changes, not to avoid them. In particular, it is interesting to trigger it before a release to make sure it will not cause too much friction.


We could also run them once a week just to check

Cron job added by d5949fa. Will run every week on Saturday midnight.

LysandreJik · 2022-11-28T13:55:40Z

contrib/requirements.txt

+pytest
+pytest-env


imo we could just require the testing extra here instead of adding another requirements.txt file.

Good call. Made the change.

LysandreJik · 2022-11-28T13:57:39Z

contrib/sentence_transformers/test_sentence_transformers.py

+@pytest.mark.xfail(
+    reason=(
+        "Production endpoint is hardcoded in sentence_transformers when pushing to Hub."
+    )
+)
+def test_push_to_hub(
+    multi_qa_model: SentenceTransformer, repo_name: str, cleanup_repo: None
+) -> None:
+    multi_qa_model.save_to_hub(repo_name)


Would be nice to eventually have a test that doesn't fail to ensure that save to hub actually works :) we could have a specific org for that, like skops does, but I understand it's a bit complex to setup + very annoying to have testing artifacts on the actual hub

Would be nice to eventually have a test that doesn't fail to ensure that save to hub actually works :)

Yes, completely agree on that. I'd like to do that later. I am about to open an issue/PR on sentence transformers side to test that properly.
Worse case scenario, I have set a reminder to myself in 10 days.

LysandreJik · 2022-11-28T13:58:19Z

contrib/timm/test_timm.py

+def test_push_to_hub(repo_name: str, cleanup_repo: None) -> None:
+    model = timm.create_model("resnet18")
+    timm.models.hub.push_to_hf_hub(model, repo_name)


Should we also test that the model pushed is according to what we expect? For example, that we can redownload it and use it once again, as this would be the usual workflow?

(discussed offline) Decision has been taken that the contrib/ test suite purpose is only to test the deprecation warnings in downstream libraries. Testing the validity of a pushed/downloaded model is therefore out of scope here.
This can be reevaluated in the future :)

osanseviero

Agree with @LysandreJik feedback, other than that it LGTM! 🔥 great work

My only concern is identifying issues to 3rd party libraries close to release rather than earlier in the process, so the more often we could run this (e.g. every week sounds great), the better

codecov · 2022-11-28T15:04:14Z

Codecov Report

Base: 84.37% // Head: 84.33% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (b148848) compared to base (22c1431).
Patch has no changes to coverable lines.

❗ Current head b148848 differs from pull request most recent head f20ab77. Consider uploading reports for the commit f20ab77 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1200      +/-   ##
==========================================
- Coverage   84.37%   84.33%   -0.04%     
==========================================
  Files          44       44              
  Lines        4365     4355      -10     
==========================================
- Hits         3683     3673      -10     
  Misses        682      682

Impacted Files	Coverage Δ
src/huggingface_hub/file_download.py	`88.09% <0.00%> (-0.35%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Wauplin · 2022-11-28T15:24:44Z

@osanseviero @LysandreJik thanks for the last reviews :)
I've made some last minute changes:

added a script to list contrib test and update Makefile/github action. It's now integrated to make style and make quality.
added a cron to run tests every Saturday midnight. Let's see how it goes. Later work should be to keep us updated about the cron result (slack message for instance ?). This can be done later as my main concern is to actually get more contrib tests first.
removed the contrib/requirements.txt in favor of .[testing]. Let's see if at some point we need a more specific requirement.

So I think we are now finally good to go 😄 🔥

Wauplin added 4 commits November 18, 2022 12:01

First draft for a contrib test suite + test for timm contrib

f22d1cb

run only Python 3.8

fc1da54

Merge branch 'main' into 1190-rfc-add-contrib-test-suite

24427d3

remove commented code

64185aa

Merge branch 'main' into 1190-rfc-add-contrib-test-suite

572ca56

osanseviero reviewed Nov 18, 2022

View reviewed changes

contrib/requirements.txt Outdated Show resolved Hide resolved

.github/workflows/contrib-tests.yml Outdated Show resolved Hide resolved

contrib/test_timm.py Outdated Show resolved Hide resolved

contrib/requirements.txt Outdated Show resolved Hide resolved

Wauplin added 7 commits November 21, 2022 18:06

Merge branch 'main' into 1190-rfc-add-contrib-test-suite

e4766a7

Run contrib tests in separate environments

af45cdb

fix ci

821aa04

fix ci again

2a0d7c6

and now ?

339af7f

stupid me

68c26b2

this time ?

1eb0a09

Refactor how to run contrib tests locally

d4a72e7

Wauplin added 2 commits November 22, 2022 12:14

add tests for sentence_transformers

aa1ffad

amke style

930c29e

osanseviero reviewed Nov 23, 2022

View reviewed changes

contrib/README.md Outdated Show resolved Hide resolved

Update contrib/README.md

625768d

Co-authored-by: Omar Sanseviero <osanseviero@gmail.com>

Wauplin requested review from julien-c and LysandreJik November 23, 2022 08:14

Merge branch 'main' into 1190-rfc-add-contrib-test-suite

7b28a5f

Wauplin changed the title ~~[RFC] First draft for a contrib test suite + test for timm contrib~~ [RFC] Contrib test suite + test for timm and sentence_transformers Nov 23, 2022

Wauplin changed the title ~~[RFC] Contrib test suite + test for timm and sentence_transformers~~ [RFC] Contrib test suite + tests for timm and sentence_transformers Nov 23, 2022

Wauplin added 2 commits November 24, 2022 15:36

Merge branch 'main' into 1190-rfc-add-contrib-test-suite

987ec8a

ADapt timm tests

12aa4ba

Wauplin linked an issue Nov 25, 2022 that may be closed by this pull request

Create a contrib/ folder with example scripts from downstream libraries #1190

Closed

osanseviero reviewed Nov 25, 2022

View reviewed changes

Wauplin added 2 commits November 25, 2022 13:19

Include feedback form osanseviero

173aff8

Merge branch 'main' into 1190-rfc-add-contrib-test-suite

cd11be8

Merge branch 'main' into 1190-rfc-add-contrib-test-suite

b148848

LysandreJik mentioned this pull request Nov 25, 2022

Remove deprecated code in v0.12 #1196

Merged

LysandreJik approved these changes Nov 28, 2022

View reviewed changes

Merge branch 'main' into 1190-rfc-add-contrib-test-suite

d8ddba6

osanseviero approved these changes Nov 28, 2022

View reviewed changes

script to check contrib list is accurate

f20ab77

Wauplin added 3 commits November 28, 2022 16:09

Use [testing] requirements as contrib common dependencies

d5949fa

add check_contrib_list in github workflow

6cb96e7

code qualiry

b299077

Wauplin mentioned this pull request Nov 28, 2022

Remove hardcoded HF endpoint UKPLab/sentence-transformers#1767

Closed

Wauplin merged commit b33c1f2 into main Nov 28, 2022

Wauplin deleted the 1190-rfc-add-contrib-test-suite branch November 28, 2022 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Contrib test suite + tests for timm and sentence_transformers #1200

[RFC] Contrib test suite + tests for timm and sentence_transformers #1200

Wauplin commented Nov 18, 2022 •

edited

Loading

HuggingFaceDocBuilderDev commented Nov 18, 2022 •

edited

Loading

osanseviero left a comment

Wauplin commented Nov 21, 2022

Wauplin commented Nov 22, 2022 •

edited

Loading

osanseviero left a comment

osanseviero Nov 25, 2022

Wauplin Nov 25, 2022 •

edited

Loading

osanseviero Nov 25, 2022

Wauplin Nov 25, 2022

Wauplin commented Nov 25, 2022

LysandreJik left a comment

LysandreJik Nov 28, 2022

LysandreJik Nov 28, 2022

Wauplin Nov 28, 2022

LysandreJik Nov 28, 2022

Wauplin Nov 28, 2022

LysandreJik Nov 28, 2022

Wauplin Nov 28, 2022

LysandreJik Nov 28, 2022

Wauplin Nov 28, 2022

LysandreJik Nov 28, 2022

Wauplin Nov 28, 2022

LysandreJik Nov 28, 2022

Wauplin Nov 28, 2022

osanseviero left a comment

codecov bot commented Nov 28, 2022

Wauplin commented Nov 28, 2022



		@contextlib.contextmanager
		def production_endpoint() -> Generator:

		4. Edit `makefile` to add the lib to `CONTRIB_LIBS` variable. Example: `CONTRIB_LIBS := timm transformers`
		5. Edit `.github/workflows/contrib-tests.yml` to add the lib to `matrix.contrib` list. Example: `contrib: ["timm", "transformers"]`


		Contrib tests can be [manually triggered in GitHub](https://github.com/huggingface/huggingface_hub/actions) with the `Contrib tests` workflow.

		Tests are not run in the default test suite (for each PR) as this would slow down development process. The goal is to notice breaking changes, not to avoid them. In particular, it is interesting to trigger it before a release to make sure it will not cause too much friction.

		pytest
		pytest-env

[RFC] Contrib test suite + tests for timm and sentence_transformers #1200

[RFC] Contrib test suite + tests for timm and sentence_transformers #1200

Conversation

Wauplin commented Nov 18, 2022 • edited Loading

How it works ?

How to add a new library ?

Run contrib tests in CI

Run contrib tests locally

HuggingFaceDocBuilderDev commented Nov 18, 2022 • edited Loading

osanseviero left a comment

Choose a reason for hiding this comment

Wauplin commented Nov 21, 2022

Wauplin commented Nov 22, 2022 • edited Loading

osanseviero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wauplin Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wauplin commented Nov 25, 2022

LysandreJik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osanseviero left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 28, 2022

Codecov Report

Wauplin commented Nov 28, 2022

Wauplin commented Nov 18, 2022 •

edited

Loading

HuggingFaceDocBuilderDev commented Nov 18, 2022 •

edited

Loading

Wauplin commented Nov 22, 2022 •

edited

Loading

Wauplin Nov 25, 2022 •

edited

Loading