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

Jump to bottom

Sentence Transformers test (soon) no longer expected to fail #1918

Merged
merged 5 commits into from
Dec 18, 2023
Merged

Sentence Transformers test (soon) no longer expected to fail #1918

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0461bfe
Sentence Transformers test (soon) no longer expected to fail
tomaarsen Dec 18, 2023
07a94e7
Merge branch 'main' into contrib/sentence_transformers
Wauplin Dec 18, 2023
26e6d61
Sleep before calling model_info
tomaarsen Dec 18, 2023
a74d62a
Merge branch 'contrib/sentence_transformers' of https://github.com/to…
tomaarsen Dec 18, 2023
ae7389c
Add sleep to spaCy tests, too
tomaarsen Dec 18, 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
11 changes: 8 additions & 3 deletions contrib/sentence_transformers/test_sentence_transformers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import pytest
from sentence_transformers import SentenceTransformer, util

from huggingface_hub import model_info

from ..utils import production_endpoint


Expand All @@ -22,6 +24,9 @@ def test_from_pretrained(multi_qa_model: SentenceTransformer) -> None:
print("Similarity:", util.dot_score(query_embedding, passage_embedding))


@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)
def test_push_to_hub(multi_qa_model: SentenceTransformer, repo_name: str, user: str, cleanup_repo: None) -> None:
multi_qa_model.save_to_hub(repo_name, organization=user)

# Check model has been pushed properly
model_id = f"{user}/{repo_name}"
assert model_info(model_id).library_name == "sentence-transformers"
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like for some reason the library_name attribute is not set when requesting the model info 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

Certainly odd. When I install with git+https://github.com/UKPLab/sentence-transformers.git#egg=sentence-transformers, it fails the first 2 times that I run pytest, and passes the 2 subsequent times :/

Copy link
Member Author

Choose a reason for hiding this comment

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

When it fails, the only siblings in ModelInfo is [RepoSibling(rfilename='.gitattributes', size=None, blob_id=None, lfs=None)], so the repo only has the gitattributes. If I breakpoint and inspect the times that it does pass, then all files are seen by the model_info.

Copy link
Contributor

Choose a reason for hiding this comment

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

and between each run, is model_id = f"{user}/{repo_name}" changing? The value should be unique for each run (generated here). I'm asking because having dependency between tests is odd

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it does differ between runs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha, now the spaCy tests are failing & sentence_transformers is passing. Perhaps it's indeed a flaky test setup where model_info gets slightly outdated data if it is called "too soon".

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @Kakulukian @coyotte508 do you know if something has changed recently on the /api/models/<repo_id> endpoint that would make the model cache longer to update? In the test and thread above, we are doing:

  1. create new repo
  2. push model with modelcard
  3. get model info (GET /api/models/repo_id)
  4. check model_info.library_name.

It looks like adding a 1 second delay between steps 2. and 3. makes the test more robust. But I don't remember this was the case before. It is not a problem to add this delay in our tests but prefer to let you know in case it's a bigger problem server-side.

Copy link
Member

@coyotte508 coyotte508 Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Now we rely on cache instead of building from scratch. So if the cache update (following the push) is going on in the background, it can display outdated info.

You can bypass this by passing the commit ID instead of HEAD, eg /api/models/<repo_id>/revision/<commit_id>

You can pass main as the commit id (for now it should work, maybe later we'll optimize)

We can add support for Cache-Control header to skip cache if needed later on

Copy link
Member

@coyotte508 coyotte508 Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Note that this seeems to happen only for commit endpoint, not push (as it's been awaited since https://github.com/huggingface/moon-landing/pull/5501)

We can fix it but potentially commit endpoint will take longer

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation! No need to optimize anything on the endpoint as it's mostly a problem in internal tests. Good to know about the revision workaround 👍

Loading