Add Hub verification token to evaluation metadata #1142

lewtun · 2022-10-31T13:13:15Z

This PR adds a new verifyToken field to the evaluation metadata schema to enable the Hub to verify whether evaluation results come from Hugging Face's evaluation service vs self-reported.

This is need to enable the following PRs:

Note: I'm not sure what's the best practice to handle camelCase fields in Python codebases, so please let me know if I should refactor variable names :)

src/huggingface_hub/repocard.py

HuggingFaceDocBuilderDev · 2022-10-31T13:21:18Z

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

lewtun · 2022-10-31T13:32:14Z

src/huggingface_hub/repocard_data.py

    verified: Optional[bool] = None

+    # Generated by Hugging Face to verify the results are valid.
+    verifyToken: Optional[str] = None


I use camelCase here because that's what's implemented on the Hub side: https://github.com/huggingface/moon-landing/pull/4037/files#diff-e9eafbf94fe033bdaf22611fa3eab89f02aa8776685889dd5cace85d7ed82257R82

@lewtun Sorry to ask for a last change but would it be possible to use snake_case here ? I think keeping consistency within the hfh parameters is more important than keeping consistency with the server naming.

This would require to change both the getter and the setter in

# in model_index_to_eval_results verify_token = metric.get("verifyToken") (...) verify_token=verify_token,

and

# in eval_results_to_model_index "verifyToken": result.verify_token,

Thanks, I agree snake_case is nicer :)

Fixed in f0d1cc4 and 6995d2a

osanseviero · 2022-10-31T13:59:24Z

Is the huggingface_hub library the best place to add support for these more internal use cases? Except us, nobody else should use this (and one could argue the same for the previous verified parameter). Looking at the PRs, I was wondering if doing something like

card.data.eval_results[0].metrics_token = "token"

or similar would make more sense, rather than supporting setting verified tokens out of the box

lewtun · 2022-10-31T14:11:26Z

Is the huggingface_hub library the best place to add support for these more internal use cases? Except us, nobody else should use this (and one could argue the same for the previous verified parameter). Looking at the PRs, I was wondering if doing something like
card.data.eval_results[0].metrics_token = "token"
or similar would make more sense, rather than supporting setting verified tokens out of the box

We currently rely on the metadata_update() function in AutoTrain to create Hub PRs for evaluation (see here), which means metadata like verifyToken is excluded unless it's included in the huggingface_hub schema.

If desired, we could roll our own version of this function using the commit API functions in huggingface_hub, although that's a bit more work on our side.

cc @abhishekkrthakur

julien-c · 2022-10-31T15:07:29Z

hmm yeah i think this should live in internal code (wdyt also @coyotte508 @allendorf?)

coyotte508 · 2022-10-31T15:19:15Z

IMO verified / verifyToken should at least be in the schema if only to be read? I mean they will be in model cards / in the JSON from the hub API (at least verified will be), I don't see why they shouldn't be in the metrics type.

Take this with a grain of salt, I'm only a little familiar with the hub library and not at all with how metadata is used in its scope.

Wauplin · 2022-10-31T15:40:20Z

IMO verified / verifyToken should at least be in the schema if only to be read? I mean they will be in model cards / in the JSON from the hub API (at least verified will be), I don't see why they shouldn't be in the metrics type.

I would be of @coyotte508 's opinion, at least if I understand correctly what this token is. For what I have read in https://github.com/huggingface/moon-landing/issues/3263 (and more precisely https://github.com/huggingface/moon-landing/issues/3263#issuecomment-1239320855) (internal links), we want to generate a token that proves the metrics have been verified by HF and we want this token to be public so that everyone can check it is a valid one, right ? If that's the case, I'd definitely be ok to have them in hfh, maybe with more explanations in the docstring part on what this token is.

lewtun · 2022-10-31T20:35:26Z

we want to generate a token that proves the metrics have been verified by HF and we want this token to be public so that everyone can check it is a valid one, right ? If that's the case, I'd definitely be ok to have them in hfh, maybe with more explanations in the docstring part on what this token is.

Yes, verifyToken is a signed JWT that enables HF to verify if a model evaluation came from our evaluation service. I'm happy to clarify this with a better docstring (I'm also equally happy to move this logic to AutoTrain if desired)

julien-c · 2022-11-01T18:20:19Z

Ok i hadn't actually read the code so i thought the signing code was here (which wouldn't make sense IMO)

Yes the types in repocard_data.py could be here indeed (ie. the actual types from the API) but metrics_token not sure if it's useful to add it here (especially confusing as it's not the name from the API)

Up to you

Wauplin · 2022-11-02T08:38:24Z

Agree that metrics_token is misleading as the only token we have at the moment is the User Access Token. But well documented I guess it should be good. I don't expect a lot of users to use this feature anyway so we can expect someone interested in it will take the time to understand how it works/read the docstring.

lewtun · 2022-11-02T10:03:26Z

Thanks for the feedback @julien-c @Wauplin ! I'll update this PR accordingly

src/huggingface_hub/repocard.py

Wauplin · 2022-11-03T11:40:05Z

(@lewtun I have just merged main in your branch to make the CI pass. You need to do a git pull locally before making any new commits)

codecov · 2022-11-03T11:54:56Z

Codecov Report

Base: 84.61% // Head: 84.63% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (06fb50b) compared to base (a02f2b9).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 06fb50b differs from pull request most recent head 514689b. Consider uploading reports for the commit 514689b to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1142      +/-   ##
==========================================
+ Coverage   84.61%   84.63%   +0.01%     
==========================================
  Files          41       41              
  Lines        4096     4100       +4     
==========================================
+ Hits         3466     3470       +4     
  Misses        630      630

Impacted Files	Coverage Δ
src/huggingface_hub/repocard.py	`93.85% <100.00%> (+0.06%)`	⬆️
src/huggingface_hub/repocard_data.py	`98.41% <100.00%> (+0.02%)`	⬆️

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.

Co-authored-by: Lucain <lucainp@gmail.com>

lewtun · 2022-11-03T12:42:10Z

@Wauplin thanks for the reviews - I think this should be good to go once the CI runs. Let me know if you want any more changes :)

Wauplin · 2022-11-03T12:53:05Z

@lewtun sorry I just added a last comment in #1142 (comment) and then we should be good to go I think. Thanks in advance 🙏

lewtun · 2022-11-04T10:54:29Z

tests/test_repocard_data.py

@@ -91,6 +91,8 @@ def test_model_index_to_eval_results(self):
                            {
                                "type": "acc",
                                "value": 0.9,
+                                "verified": True,


I found it useful to add this info to the unit test to be sure that these new fields are behaving as expected

lewtun · 2022-11-04T10:55:26Z

@Wauplin thanks for the snake_case feedback - should now be fixed and ready to go if the CI passes :)

Wauplin · 2022-11-04T11:06:00Z

Perfect ! Thanks @lewtun for taking care of all the details :) 🔥

lewtun added 3 commits October 31, 2022 14:10

Add Hub verification token to evaluation metadata

e3a44b4

Fix formatting

2c1dee6

Fix formatting

4807587

lewtun commented Oct 31, 2022

View reviewed changes

src/huggingface_hub/repocard.py Outdated Show resolved Hide resolved

Fix formatting

3406ca8

lewtun commented Oct 31, 2022

View reviewed changes

src/huggingface_hub/repocard.py Outdated Show resolved Hide resolved

lewtun commented Oct 31, 2022

View reviewed changes

Add deprecation & improve docstring

0e1b34d

lewtun commented Nov 3, 2022

View reviewed changes

src/huggingface_hub/repocard.py Show resolved Hide resolved

lewtun added 2 commits November 3, 2022 12:08

Fix docstring

4b4b87a

Fix

f672672

Wauplin reviewed Nov 3, 2022

View reviewed changes

src/huggingface_hub/repocard.py Outdated Show resolved Hide resolved

src/huggingface_hub/repocard.py Show resolved Hide resolved

Merge branch 'main' into add-verifytoken

5bc4f53

lewtun and others added 2 commits November 3, 2022 13:22

Fix positional args

2a56560

Co-authored-by: Lucain <lucainp@gmail.com>

Fix

06fb50b

Use snake_case

f0d1cc4

lewtun added 2 commits November 4, 2022 11:46

Fix snake_case

6995d2a

Add test case

514689b

lewtun commented Nov 4, 2022

View reviewed changes

Wauplin approved these changes Nov 4, 2022

View reviewed changes

Wauplin merged commit 91fe43c into main Nov 4, 2022

Wauplin deleted the add-verifytoken branch November 4, 2022 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Hub verification token to evaluation metadata #1142

Add Hub verification token to evaluation metadata #1142

lewtun commented Oct 31, 2022 •

edited

Loading

HuggingFaceDocBuilderDev commented Oct 31, 2022 •

edited

Loading

lewtun Oct 31, 2022

Wauplin Nov 3, 2022

lewtun Nov 4, 2022

osanseviero commented Oct 31, 2022

lewtun commented Oct 31, 2022

julien-c commented Oct 31, 2022

coyotte508 commented Oct 31, 2022 •

edited

Loading

Wauplin commented Oct 31, 2022

lewtun commented Oct 31, 2022

julien-c commented Nov 1, 2022

Wauplin commented Nov 2, 2022 •

edited

Loading

lewtun commented Nov 2, 2022

Wauplin commented Nov 3, 2022

codecov bot commented Nov 3, 2022 •

edited

Loading

lewtun commented Nov 3, 2022

Wauplin commented Nov 3, 2022

lewtun Nov 4, 2022

lewtun commented Nov 4, 2022

Wauplin commented Nov 4, 2022 •

edited

Loading

Add Hub verification token to evaluation metadata #1142

Add Hub verification token to evaluation metadata #1142

Conversation

lewtun commented Oct 31, 2022 • edited Loading

HuggingFaceDocBuilderDev commented Oct 31, 2022 • edited Loading

lewtun Oct 31, 2022

Choose a reason for hiding this comment

Wauplin Nov 3, 2022

Choose a reason for hiding this comment

lewtun Nov 4, 2022

Choose a reason for hiding this comment

osanseviero commented Oct 31, 2022

lewtun commented Oct 31, 2022

julien-c commented Oct 31, 2022

coyotte508 commented Oct 31, 2022 • edited Loading

Wauplin commented Oct 31, 2022

lewtun commented Oct 31, 2022

julien-c commented Nov 1, 2022

Wauplin commented Nov 2, 2022 • edited Loading

lewtun commented Nov 2, 2022

Wauplin commented Nov 3, 2022

codecov bot commented Nov 3, 2022 • edited Loading

Codecov Report

lewtun commented Nov 3, 2022

Wauplin commented Nov 3, 2022

lewtun Nov 4, 2022

Choose a reason for hiding this comment

lewtun commented Nov 4, 2022

Wauplin commented Nov 4, 2022 • edited Loading

lewtun commented Oct 31, 2022 •

edited

Loading

HuggingFaceDocBuilderDev commented Oct 31, 2022 •

edited

Loading

coyotte508 commented Oct 31, 2022 •

edited

Loading

Wauplin commented Nov 2, 2022 •

edited

Loading

codecov bot commented Nov 3, 2022 •

edited

Loading

Wauplin commented Nov 4, 2022 •

edited

Loading