Skip to content
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

Use human readable name vs underscore name #135

Merged
merged 2 commits into from
Mar 1, 2024
Merged

Use human readable name vs underscore name #135

merged 2 commits into from
Mar 1, 2024

Conversation

mkly
Copy link
Member

@mkly mkly commented Feb 29, 2024

  • Use human readable name vs underscore name for sut display name

I wasn't fully sure this was the correct value to use. While "Meta Llama 2, 7b parameters" feels better than "LLAMA_2_7B" for a human readable title, I almost feel like maybe we add one more value to the tuple that is something like "Llama 2 7B" as it feels a little long. "Llama 2 7B" is the key with the underscores removed so another option is a computed value that just title cases and removes underscores. All are good to me. Just want something without underscores for the general public.

* Use human readable name vs underscore name for sut display name
Copy link

github-actions bot commented Feb 29, 2024

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@mkly mkly requested review from wpietri and dhosterman February 29, 2024 22:09
@dhosterman
Copy link
Collaborator

I think we probably need some feedback from the users before we settle on a real solution here, but I'd probably be more inclined to add a display value than try to compute one. @wpietri , what are your thoughts?

@mkly
Copy link
Member Author

mkly commented Mar 1, 2024

Sounds great. I'm not sure which of the options is best outside of the underscores is obviously incorrect. Outside of that it starts to bikeshed a bit for me so happy with anything else. Thank you.

@wpietri
Copy link
Contributor

wpietri commented Mar 1, 2024

If the name value isn't returning what we want for a user-friendly name, we should change what that returns, as that's its only purpose. What I typed into the fields in the NewhelmSut enum were just my best guesses at a human-friendly name. Since our target audience here non-expert users, I think the names should contextualize things for that audience, which is why I went with "Meta Llama 2" over "Llama 2", but I'm find with making it "7B" rather than "7b parameters". Just edit the enum fields to be whatever you like.

@dhosterman
Copy link
Collaborator

If the name value isn't returning what we want for a user-friendly name, we should change what that returns, as that's its only purpose. What I typed into the fields in the NewhelmSut enum were just my best guesses at a human-friendly name. Since our target audience here non-expert users, I think the names should contextualize things for that audience, which is why I went with "Meta Llama 2" over "Llama 2", but I'm find with making it "7B" rather than "7b parameters". Just edit the enum fields to be whatever you like.

Perfect, I wasn't sure if there was some hidden other use case for the name value. Seems easy enough then!

@mkly
Copy link
Member Author

mkly commented Mar 1, 2024

Thank you for all this additional info. I did not realize the name was already there as display_name. I've changed this commit to use display_name which feels like what it is meant for. Let me know if that works. I added a quick test to illustrate.

@bollacker
Copy link
Collaborator

I have some opinions about this, having had to model this exact situation a 100 times over 15 years. I usually avoid the "name" field. Instead, there should be two fields:

  1. Display Name (human consumption only)
  2. UID (e.g. URN, for machine consumption, but can be semi-human readable)

One can generate one of these fields from the other, but often is is better to do them separately (e.g. You might have multiple Display Names in different languages)

I am trying to stay out of most technical design decisions, but IDs are something I know and care a lot about.

@mkly
Copy link
Member Author

mkly commented Mar 1, 2024

Just following up from the discussion. display_name is now used and I believe we have decided not to add uid for now. If we would like to do that, let me know and I can add it. Thanks

Copy link
Contributor

@wpietri wpietri left a comment

Choose a reason for hiding this comment

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

Seems good to me!

@mkly mkly merged commit e83eb8d into main Mar 1, 2024
2 checks passed
@mkly mkly deleted the mkly/sut-name-1 branch March 1, 2024 21:13
@github-actions github-actions bot locked and limited conversation to collaborators Mar 1, 2024
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.

4 participants