-
Notifications
You must be signed in to change notification settings - Fork 45
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.
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
Add schema.org sdLicense #545
base: main
Are you sure you want to change the base?
Conversation
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
c85ea2c
to
cf0437a
Compare
cf0437a
to
94ed732
Compare
https://schema.org/sdLicense is in the spec now. I set the sdLicenses to Apache 2.0, since the repository is Apache 2.0. sdLicense is either None or string for now. Note: migration for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just wrote a few comments.
Also, do you want to use apache-2.0
(Hugging Face way) or a URL (Croissant way described in the specs)?
Croissant recommends using the URL of a known license, e.g., one of the licenses listed at https://spdx.org/licenses/.
...n/mlcroissant/mlcroissant/_src/tests/graphs/0.8/metadata_missing_property_name/metadata.json
Outdated
Show resolved
Hide resolved
327af2c
to
7b75ba9
Compare
7b75ba9
to
50ac1e8
Compare
@marcenacp The migration tool applies these changes even to v0.8, so I manually merged the diffs. Also, I think the migration tool may reorder the alphabetical order differently than a JSON formatter/linter, but this seems minor. |
@marcenacp Is there something that would block the merge if I rebased onto the current main? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that documentation is required to explain in what the fields license
and sdLicense
differ. Do they accept labels/strings or URIs?
@Zack-83 They are schema.org fields so you can find the documentation on respectively https://schema.org/license and https://schema.org/sdLicense. Does this make sense to you? We could explicitly point to those URLs. @mkuchnik Nothing blocks the merge on my side. Do you have all the rights for merge? |
@@ -265,6 +268,17 @@ def validate_license(self) -> list[str] | None: | |||
self.add_error(f"License should be a list of str. Got: {license}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.add_error(f"License should be a list of str. Got: {license}") | |
self.add_error(f"The object license should be a list of strings. Got: {license}") |
elif isinstance(sd_license, str): | ||
return sd_license | ||
else: | ||
self.add_error(f"sdLicense should be a str. Got: {sd_license}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.add_error(f"sdLicense should be a str. Got: {sd_license}") | |
self.add_error(f"The license for the metadata (schema:sdLicense) should be a string. Got: {sd_license}") |
@marcenacp thanks for considering my suggestion. I suppose that it would be useful to reference the URLs or disambiguate the two terms in another way at whatever position where the user inputs the license manually or where he/she gets some output, e.g. in the error messages of metadata.py. |
Add a field for the metadata's license
See #544