ENH Add update metadata to repocard #844

lvwerra · 2022-04-19T10:09:10Z

This PR adds a metadata_update function that allows the user to update the metadata in a repository on the hub. The function accepts a dict with metadata (following the same pattern as the YAML in the README) and behaves as follows for all top level fields except model-index:

if field in existing README does not exist it is added.
if it exists an error is thrown except if overwrite=True is passed as a safety guard.

For model-index the behaviour is more nuanced:

if an entry with the same task and dataset exist, then
- if the same metric type/name does not exist the metric is appended to the list
- if the same metric type/name exists the value is overwritten (given overwrite=True)
if an entry with the same task and dataset does not exist, the result is appended to the results

For reference this is an example of a model's metadata structure as a dictionary:

{'datasets': ['lvwerra/codeparrot-clean-train'],
 'language': 'code',
 'model-index': [{'name': 'codeparrot',
                  'results': [{'dataset': {'name': 'HumanEval',
                                           'type': 'openai_humaneval'},
                               'metrics': [{'name': 'pass@1',
                                            'type': 'code_eval',
                                            'value': 3.99},
                                           {'name': 'pass@10',
                                            'type': 'code_eval',
                                            'value': 8.69},
                                           {'name': 'pass@100',
                                            'type': 'code_eval',
                                            'value': 17.88}],
                               'task': {'name': 'Code Generation',
                                        'type': 'code-generation'}}]}],
 'tags': ['code', 'gpt2', 'generation'],
 'widget': [{'example_title': 'Transformers',
             'text': 'from transformer import'},
            {'example_title': 'Hello World!',
             'text': 'def print_hello_world():\n\t'},
            {'example_title': 'File size',
             'text': 'def get_file_size(filepath):'},
            {'example_title': 'Numpy', 'text': 'import numpy as'}]}

One minor issue I found is that I need to use force_download=True for the tests to pass as otherwise the hf_hub_download uses the cached but outdated version of the README even if the README has been updated on the remote. cc @LysandreJik

huggingface_hub/src/huggingface_hub/repocard.py

Line 137 in a191950

force_download=True,

This feature will be used for huggingface/evaluate#6 and closes #835.

HuggingFaceDocBuilderDev · 2022-04-19T10:18:01Z

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

julien-c · 2022-04-19T13:10:15Z

the behavior sounds reasonable to me, but isn't it weird in terms of API design that there's a special case for model-index? (maybe not)

lvwerra · 2022-04-19T13:23:31Z

The main reason to do it different for model-index is usability: if we take the same approach as for the other fields one would need to always pass all the existing results/metrics too, even if for just updating/adding a single entry.

Alternatively, we could outsource that logic to a helper function that grabs the existing model-index and updates it with the new results and then one can pass the complete (existing+new) model-index to metadata_update.

adrinjalali

Thanks for the PR @lvwerra .

In the future, please send PRs from your own branch. We're trying to cleanup branches on the main repo. The CI works here on PRs from fork repositories.

src/huggingface_hub/repocard.py

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

lvwerra · 2022-04-21T16:09:59Z

Code Update

I refactored the _update_metadata_model_index function:

outsourced the inner for-loops
documented the functions
added the unique identifying features as a configurable list since they might change (e.g. adding dataset args or configs)

Cases

Given the following example for existing metadata on the hub under model-index there are three cases:

existing_results = [{'dataset': {'name': 'IMDb', 'type': 'imdb'},
                                 'metrics': [{'name': 'Accuracy', 'type': 'accuracy', 'value': 0.995}],
                                 'task': {'name': 'Text Classification', 'type': 'text-classification'}}]

1 Overwrite existing metric value in existing result

This happens if the values of 'dataset' and 'task' are equal as well as 'name' and 'value' of the metric. This requires overwrite=True otherwise throws ValueError:

new_results = deepcopy(existing_results)
new_results[0]["metrics"][0]["value"] = 0.999
_update_metadata_model_index(existing_results, new_results, overwrite=True)

# result:
[{'dataset': {'name': 'IMDb', 'type': 'imdb'},
  'metrics': [{'name': 'Accuracy', 'type': 'accuracy', 'value': 0.999}],
  'task': {'name': 'Text Classification', 'type': 'text-classification'}}]

2 Add new metric to existing result

This happens if the values of 'dataset' and 'task' are equal but 'name' and 'value' of the metric are not:

new_results = deepcopy(existing_results)
new_results[0]["metrics"][0]["name"] = "Recall"
new_results[0]["metrics"][0]["type"] = "recall"

# result:
[{'dataset': {'name': 'IMDb', 'type': 'imdb'},
  'metrics': [{'name': 'Accuracy', 'type': 'accuracy', 'value': 0.995},
              {'name': 'Recall', 'type': 'recall', 'value': 0.995}],
  'task': {'name': 'Text Classification', 'type': 'text-classification'}}]

3 Add new result

This happens if not both values of 'dataset' and 'task' match with the new result:

new_results = deepcopy(existing_results)
new_results[0]["dataset"] = {'name': 'IMDb-2', 'type': 'imdb_2'}

# result:
[{'dataset': {'name': 'IMDb', 'type': 'imdb'},
  'metrics': [{'name': 'Accuracy', 'type': 'accuracy', 'value': 0.995}],
  'task': {'name': 'Text Classification', 'type': 'text-classification'}},
 {'dataset': ({'name': 'IMDb-2', 'type': 'imdb_2'},),
  'metrics': [{'name': 'Accuracy', 'type': 'accuracy', 'value': 0.995}],
  'task': {'name': 'Text Classification', 'type': 'text-classification'}}]

Hope that clarifies what _update_metadata_model_index is supposed to do.

src/huggingface_hub/repocard.py

adrinjalali · 2022-04-25T09:27:02Z

src/huggingface_hub/repocard.py

+    for new_result in new_results:
+        result_found = False
+        for existing_result_index, existing_result in enumerate(existing_results):
+            if all(
+                new_result[feat] == existing_result[feat]
+                for feat in UNIQUE_RESULT_FEATURES
+            ):
+                result_found = True
+                existing_results[existing_result_index][
+                    "metrics"
+                ] = _update_metadata_results_metric(
+                    new_result["metrics"],
+                    existing_result["metrics"],
+                    overwrite=overwrite,
+                )
+        if not result_found:
+            existing_results.append(new_result)


Since dictionaries are mutable, you could also write this as:

try: existing_result = next( x for x in existing_results if all(x[feat] == new_result[feat] for feat in UNIQUE_RESULT_FEATURES) ) existing_result["metrics"] = _update_metadata_results_metric( new_result["metrics"], existing_result["metrics"], overwrite=overwrite, ) except StopIteration: existing_results.append(new_result)

which should be faster since it avoids one slow for loop.

Added this.

src/huggingface_hub/repocard.py

adrinjalali · 2022-04-25T09:39:22Z

src/huggingface_hub/repocard.py

+    for new_metric in new_metrics:
+        metric_exists = False
+        for existing_metric_index, existing_metric in enumerate(existing_metrics):
+            if all(
+                new_metric[feat] == existing_metric[feat]
+                for feat in UNIQUE_METRIC_FEATURES
+            ):
+                if overwrite:
+                    existing_metrics[existing_metric_index]["value"] = new_metric[
+                        "value"
+                    ]
+                else:
+                    # if metric exists and value is not the same throw an error without overwrite flag
+                    if (
+                        existing_metrics[existing_metric_index]["value"]
+                        != new_metric["value"]
+                    ):
+                        raise ValueError(
+                            f"""You passed a new value for the existing metric '{new_metric["name"]}'. Set `overwrite=True` to overwrite existing metrics."""
+                        )
+                metric_exists = True
+        if not metric_exists:
+            existing_metrics.append(new_metric)


Could do the same here:

for new_metric in new_metrics: try: existing_metric = next( x for x in existing_metrics if all(x[feat] == new_metric[feat] for feat in UNIQUE_METRIC_FEATURES) ) if overwrite: existing_metric["value"] = new_metric["value"] else: # if metric exists and value is not the same throw an error # without overwrite flag if existing_metric["value"] != new_metric["value"]: existing_str = ",".join( new_metric[feat] for feat in UNIQUE_METRIC_FEATURES ) raise ValueError( "You passed a new value for the existing metric" f" '{existing_str}'. Set `overwrite=True` to overwrite existing" " metrics." ) except StopIteration: existing_metrics.append(new_metric)

This as well :)

tests/test_repocard.py

adrinjalali · 2022-04-25T09:47:57Z

tests/test_repocard.py

+        if os.path.exists(REPO_NAME):
+            shutil.rmtree(REPO_NAME, onerror=set_write_permission_and_retry)


could we clone the repo under a tempfile.mkdtemp?

Also integrated tempfile.mkdtemp everywhere.

tests/test_repocard.py

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

adrinjalali

LGTM, could you please merge with the latest main and run black on these files again? Otherwise LGTM.

lvwerra · 2022-04-26T20:52:19Z

Done - thanks for reviewing and the helpful suggestions @adrinjalali!

adrinjalali · 2022-04-27T08:03:49Z

Nice. I'll wait for @osanseviero to have a look and merge.

osanseviero

This looks very neat! I left some minor suggestions and a couple of questions. Thanks for this PR 🔥 🔥

I wrote a colab while I was exploring this https://colab.research.google.com/drive/1fG8OWYTnI6ucnafYKtrf-HwxtWbVBVQh?usp=sharing

src/huggingface_hub/repocard.py

osanseviero · 2022-05-03T12:42:38Z

src/huggingface_hub/repocard.py

+            existing_result = next(
+                x
+                for x in existing_results
+                if all(x[feat] == new_result[feat] for feat in UNIQUE_RESULT_FEATURES)


This assumes metadata in existing data is valid and has both datasets and task tags, it will break if it's not valid. How should we handle those scenarios? Should we validate model-index beforehand? Show an error message? Override invalid metadata?

Since we pull the metadata from the Hub can't we assume that it is valid? In your experiment pushing invalid metadata was rejected, right? So it should not be there in the first place if it is invalid.

Hmm @julien-c WDYT? https://huggingface.co/osanseviero/llama-horse-zebra

The result has task and metrics, but no associated dataset. This is not rejected by the server, and the evaluation results are nicely shown, with the only con that there is no associated dataset, so there's no leaderboard.

IMO this is still valid metadata which is just incomplete. Most spaCy models are like this https://huggingface.co/models?library=spacy

yes we try to still display the eval results in this case even though it's not perfect

src/huggingface_hub/repocard.py

osanseviero · 2022-05-03T13:16:21Z

src/huggingface_hub/repocard.py

+        except StopIteration:
+            existing_results.append(new_result)


Is this really needed? I think try/except to do this is a bit less readable and could be more prone to introduce bugs. Maybe the for loop could be more explicit. I think that was how it was before, I personally find that more readable and easier to maintain.

I have no strong opinion here - happy to revert. @adrinjalali?

The for loop is slower, and try/except clauses are quite pythonic. I do think the current state is quite better than a for loop. One can always add a bit of comment on when the exception is raised if that helps with readability.

I don't think speed is important here as we iterate through a dictionary with a handful of entries while pulling/pushing to the hub in the same function which is probably 1000x slower.

I would not optimize for execution speed of an update method that won't be called thousands in time per second at the moment, and would rather optimize for readability and consistency with the rest of the codebase. This implem iterates over new_results with a for loop, and over existing_results with a try/except/next approach. Should we start changing all nested loops to match this?

If you feel strong about this let's go with it though 😄

I do find the for/loop implementation less readable. Removing the outer loop would make it quite complicated and not very readable, that's why I didn't suggest to remove it.

As for consistency, I don't think it's about staying consistent with the rest of the code. Sometimes a for loop makes sense, sometimes it doesn't. And as a team when we work on a codebase, sometimes we find better ways to do things, and that's fine, and I don't think we should hold back because we have done things in a certain way so far. To me it's okay to look at a codebase and notice old stuff vs new stuff. The way people do things changes and new code can look different than the old code and that's fine to me.

But if both of you think the for loop is more readable than this, then sure, change it. To me it's the other way around.

src/huggingface_hub/repocard.py

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

lvwerra · 2022-05-03T15:20:09Z

I integrated the feedback and reverted back to for-loops for now. Can change it again if somebody has strong opinions. Let me know if this looks good now @osanseviero @adrinjalali.

osanseviero

Looks good! Let's wait until other tests (unrelated to this PR ) to be fixed to submit on green 🚀

Thanks!

tests/test_repocard.py

src/huggingface_hub/repocard.py

julien-c

left a few small nits, other than that looks good to me!

LysandreJik

Looks good! Only left nits.

LysandreJik · 2022-04-20T22:37:29Z

src/huggingface_hub/repocard.py

+        repo_type (`str`, *optional*):
+            Set to `"dataset"` or `"space"` if updating to a dataset or space,
+            `None` or `"model"` if updating to a model. Default is `None`.
+        overwrite (`bool`, *optional*):


Suggested change

overwrite (`bool`, *optional*):

overwrite (`bool`, *optional*, defaults to `False`):

src/huggingface_hub/repocard.py

Co-authored-by: Julien Chaumond <julien@huggingface.co> Co-authored-by: Lysandre Debut <lysandre.debut@reseau.eseo.fr>

LysandreJik · 2022-05-09T17:39:24Z

Thank you for addressing all comments! Merging.

* add `metadata_update` function * add tests * add docstring * Apply suggestions from code review Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> * refactore `_update_metadata_model_index` * Apply suggestions from code review Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> * fix style and imports * switch to deepcopy everywhere * load repo in repocard test into tmp folder * simplify results and metrics checks when updating metadata * run black * Apply suggestions from code review Co-authored-by: Omar Sanseviero <osanseviero@gmail.com> * fix pyyaml version to work with `sort_keys` kwarg * don't allow empty commits if file hasn't changed * switch order of updates to first check model-index for easier readbility * expose repocard functions through `__init__` * fix init * make style & quality * revert to for-loop * Apply suggestions from code review Co-authored-by: Julien Chaumond <julien@huggingface.co> Co-authored-by: Lysandre Debut <lysandre.debut@reseau.eseo.fr> * post suggestion fixes * add example * add type to list Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Omar Sanseviero <osanseviero@gmail.com> Co-authored-by: Julien Chaumond <julien@huggingface.co> Co-authored-by: Lysandre Debut <lysandre.debut@reseau.eseo.fr>

leandro added 3 commits April 19, 2022 11:20

add metadata_update function

f848a9d

add tests

f22f416

add docstring

a191950

lvwerra requested review from julien-c and osanseviero April 19, 2022 10:09

adrinjalali reviewed Apr 21, 2022

View reviewed changes

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

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

src/huggingface_hub/repocard.py Show resolved Hide resolved

src/huggingface_hub/repocard.py Show resolved Hide resolved

adrinjalali changed the title ~~Add update metadata~~ ENH Add update metadata to repocard Apr 21, 2022

lvwerra and others added 2 commits April 21, 2022 13:43

Apply suggestions from code review

98a412f

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

refactore _update_metadata_model_index

4276d21

adrinjalali reviewed Apr 25, 2022

View reviewed changes

lvwerra and others added 5 commits April 26, 2022 12:07

Apply suggestions from code review

dea3b67

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

fix style and imports

707224c

switch to deepcopy everywhere

19efcdc

load repo in repocard test into tmp folder

681dd68

simplify results and metrics checks when updating metadata

60252bd

adrinjalali reviewed Apr 26, 2022

View reviewed changes

leandro added 2 commits April 26, 2022 22:49

Merge branch 'main' into add-update-metadata

144bb21

run black

8af7cb0

osanseviero reviewed May 3, 2022

View reviewed changes

lvwerra and others added 5 commits May 3, 2022 15:27

Apply suggestions from code review

a7bf64e

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

fix pyyaml version to work with sort_keys kwarg

8b3a3ac

don't allow empty commits if file hasn't changed

b8faa73

switch order of updates to first check model-index for easier readbility

46093d1

expose repocard functions through __init__

3f00168

leandro added 3 commits May 3, 2022 17:08

fix init

3a56a04

make style & quality

e7185f6

revert to for-loop

d4fa5af

osanseviero approved these changes May 3, 2022

View reviewed changes

LysandreJik added this to the v0.6 milestone May 9, 2022

LysandreJik self-requested a review May 9, 2022 12:20

julien-c reviewed May 9, 2022

View reviewed changes

tests/test_repocard.py Outdated Show resolved Hide resolved

julien-c reviewed May 9, 2022

View reviewed changes

src/huggingface_hub/repocard.py Show resolved Hide resolved

julien-c reviewed May 9, 2022

View reviewed changes

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

julien-c reviewed May 9, 2022

View reviewed changes

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

julien-c reviewed May 9, 2022

View reviewed changes

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

julien-c reviewed May 9, 2022

View reviewed changes

LysandreJik approved these changes May 9, 2022

View reviewed changes

lvwerra and others added 4 commits May 9, 2022 18:49

Apply suggestions from code review

00dde91

Co-authored-by: Julien Chaumond <julien@huggingface.co> Co-authored-by: Lysandre Debut <lysandre.debut@reseau.eseo.fr>

post suggestion fixes

61da3a9

add example

4f57a37

add type to list

af2cf73

LysandreJik merged commit f6343cb into main May 9, 2022

LysandreJik deleted the add-update-metadata branch May 9, 2022 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Add update metadata to repocard #844

ENH Add update metadata to repocard #844

lvwerra commented Apr 19, 2022

HuggingFaceDocBuilderDev commented Apr 19, 2022 •

edited

Loading

julien-c commented Apr 19, 2022

lvwerra commented Apr 19, 2022

adrinjalali left a comment

lvwerra commented Apr 21, 2022

adrinjalali Apr 25, 2022

lvwerra Apr 26, 2022

adrinjalali Apr 25, 2022

lvwerra Apr 26, 2022

adrinjalali Apr 25, 2022

lvwerra Apr 26, 2022

adrinjalali left a comment

lvwerra commented Apr 26, 2022

adrinjalali commented Apr 27, 2022

osanseviero left a comment

osanseviero May 3, 2022

lvwerra May 3, 2022

osanseviero May 3, 2022

julien-c May 9, 2022

osanseviero May 3, 2022

lvwerra May 3, 2022

adrinjalali May 3, 2022

lvwerra May 3, 2022

osanseviero May 3, 2022

adrinjalali May 3, 2022

lvwerra commented May 3, 2022

osanseviero left a comment

julien-c left a comment

LysandreJik left a comment

LysandreJik Apr 20, 2022

LysandreJik commented May 9, 2022

		if os.path.exists(REPO_NAME):
		shutil.rmtree(REPO_NAME, onerror=set_write_permission_and_retry)

	overwrite (`bool`, optional):
	overwrite (`bool`, optional, defaults to `False`):

ENH Add update metadata to repocard #844

ENH Add update metadata to repocard #844

Conversation

lvwerra commented Apr 19, 2022

HuggingFaceDocBuilderDev commented Apr 19, 2022 • edited Loading

julien-c commented Apr 19, 2022

lvwerra commented Apr 19, 2022

adrinjalali left a comment

Choose a reason for hiding this comment

lvwerra commented Apr 21, 2022

Code Update

Cases

1 Overwrite existing metric value in existing result

2 Add new metric to existing result

3 Add new result

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

adrinjalali left a comment

Choose a reason for hiding this comment

lvwerra commented Apr 26, 2022

adrinjalali commented Apr 27, 2022

osanseviero 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

lvwerra commented May 3, 2022

osanseviero left a comment

Choose a reason for hiding this comment

julien-c left a comment

Choose a reason for hiding this comment

LysandreJik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LysandreJik commented May 9, 2022

HuggingFaceDocBuilderDev commented Apr 19, 2022 •

edited

Loading