Repository navigation
Conversation
Task metadata validation now fails when a license is missing or is a placeholder such as "not specified". Tasks that already use one stay on an exception list in the metadata tests so the current benchmark keeps passing.
Pydantic already rejects a license that is not in the allowed list or a URL. The check now only fails when the license is missing or "not specified".
|
Thanks @ayush1298 and @Samoed, that makes sense. Since pydantic already rejects anything that isn't in |
Yep a separate file sounds good. Thanks. |
The list of existing tasks with an unclear license is long, so it now lives in tests/test_tasks/_tasks_with_unclear_license.py and test_metadata.py imports it.
|
Thanks @isaac-chung. I moved the list of existing tasks into |
|
Any particular reason it's prefixed with |
no strong reason, just to mark it as a helper and not a test. |
| # Grandfathered tasks still need valid languages. Their license is an | ||
| # existing exception and is checked by the license policy below. | ||
| if task.metadata.name in TASKS_WITH_UNCLEAR_LICENSE: | ||
| task.metadata._eval_langs_are_valid(task.metadata.eval_langs) | ||
| _assert_license_policy(task) | ||
| else: | ||
| task.metadata._validate_metadata() |
There was a problem hiding this comment.
I think you can test this after this check, because this would skip a lot of tasks in _HISTORIC_DATASETS
|
|
||
| def _validate_metadata(self) -> None: | ||
| self._eval_langs_are_valid(self.eval_langs) | ||
| self._validate_license() |
There was a problem hiding this comment.
This would create a lot new unvalidating metadata which can be a bit unexpected. I think we can remove _validate_license from _validate_metadata
This change does not add a model or a dataset, so the dataset and model checklists do not apply.
New tasks need a concrete license. The
licensefield is already typed asLicenses | StrURL | None, so pydantic rejects anything that is not a known license id or a URL. The only unclear values that get through areNoneand "not specified", and this PR targets those two.What changed:
TaskMetadatagetshas_unclear_license(), which is true when the license isNoneor "not specified", and_validate_license(), which raises aValueErrorin that case._validate_metadata()does not call_validate_license().Noneand "not specified" still pass metadata validation, so existing task files keep loading and nothing new starts failing at runtime.test_all_metadata_is_filled_and_validcalls_assert_license_policy(task)for every task after the historic datasets check. A task on the exception list must still have an unclear license (otherwise it should be removed from the list), and any other task must pass_validate_license().test_unclear_licenses_limited_to_existing_tasksruns the same check over all tasks, including superseded, aggregate, beta and private ones, and fails if the exception list names a task that no longer exists.test_task_metadata.pycoverNoneand "not specified" passing_validate_metadata(), the same two values being rejected by_validate_license(), and pydantic rejecting a value like "unknown".552 existing tasks that use "not specified" are listed in
tests/test_tasks/_tasks_with_unclear_license.py. Their license values are unchanged. The file says not to add new tasks and to remove a task once it has a real license.The dataset contribution guide and the
licensedocstring now say a concrete license is required for new tasks, and the dataset PR checklist has an item for it.Fixes #5348
@KennethEnevoldsen @isaac-chung this follows what you both suggested on the issue, plus the review feedback from @Samoed and @ayush1298. Happy to change how the existing tasks are handled if you'd prefer something else.