Skip to content
This repository has been archived by the owner on Dec 15, 2023. It is now read-only.

Add CLEVA Schema #43

Merged
merged 10 commits into from
Sep 26, 2023
Merged

Add CLEVA Schema #43

merged 10 commits into from
Sep 26, 2023

Conversation

lyy1994
Copy link
Owner

@lyy1994 lyy1994 commented Sep 22, 2023

This PR adds CLEVA schema to schema.yaml according to the scaffolding of this HELM PR.

@lyy1994 lyy1994 self-assigned this Sep 22, 2023
src/helm/benchmark/static/schema.yaml Outdated Show resolved Hide resolved
when: "?"
what: e-commercial
who: customers and representatives
when: n/a
Copy link
Owner Author

Choose a reason for hiding this comment

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

Maybe an unknown "?" is more appropriate?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think "?" stands for unknown and "n/a" means this field is not necessary. In most cases, I believe we just don't know "who" wrote "what" at "when", but it does not mean that these fields are not needed. Correct me if I am wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand your point of view, and I will gladly accept it if you insist. In my opinion, I feel like "?" means this field is missing and will be filled later after some examinations, while "n/a" means the creation time of the data does not matter too much, and the data is not time-sensitive.

I can do either way. Please let me know what you think. Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can also change the value to something like "not specified" to deliver a more clear indication.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Perhaps we can stay this way and let the HELM team decide.

Copy link
Owner Author

Choose a reason for hiding this comment

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

They should have some guidelines on how to fill out this schema.yaml in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure 😊

src/helm/benchmark/static/schema.yaml Outdated Show resolved Hide resolved
src/helm/benchmark/static/schema.yaml Outdated Show resolved Hide resolved
src/helm/benchmark/static/schema.yaml Outdated Show resolved Hide resolved
src/helm/benchmark/static/schema.yaml Outdated Show resolved Hide resolved
src/helm/benchmark/static/schema.yaml Outdated Show resolved Hide resolved
src/helm/benchmark/static/schema.yaml Outdated Show resolved Hide resolved
@Jianqiao-Zhao
Copy link
Collaborator

I push my code to reflect all the discussions above. Have a look. Thanks!

Copy link
Owner Author

@lyy1994 lyy1994 left a comment

Choose a reason for hiding this comment

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

I have no other comments. Good job! I think this PR is ready to be merged and @Jianqiao-Zhao could review other parts. Thanks.

@lyy1994 lyy1994 marked this pull request as ready for review September 26, 2023 04:49
@Jianqiao-Zhao Jianqiao-Zhao merged commit ac5f025 into main Sep 26, 2023
3 checks passed
@Jianqiao-Zhao Jianqiao-Zhao deleted the schema branch September 26, 2023 04:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants