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.

Sign up for GitHub

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

Don't change the casing of model parameters. #251

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+1 −1
Open

Don't change the casing of model parameters. #251

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1905cf2
Don't change the casing of model parameters.
nicolasnoble Jan 23, 2025
00dd034
Changing the case of the type element only, instead of the whole para…
nicolasnoble Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/command_line_parser.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1648,10 +1648,10 @@ CLParser::ParseCommandLine(int argc, char** argv)
"<name:value:type>.");
}

std::for_each(values.begin(), values.end(), ToLowerCase);
std::string name{values[0]};
std::string value{values[1]};
std::string type{values[2]};
Copy link
Contributor

@nv-hwoo nv-hwoo Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It seems like we used to string match the type value but that part has been removed from the command_line_parser.cc and moved to other places in client repo so I think the lowercasing is still needed. But I think we only need to lowercase the type value (not exactly sure why I was applying it to other ones as well):

Suggested change
std::string type{values[2]};
std::string type{values[2]};
ToLowerCase(type); // to string-match the type

Copy link
Contributor

Choose a reason for hiding this comment

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

After giving it some more thoughts, I think another main issue here is that the place where the lowercasing is actually being used is now very far away. So I'm leaning into just deleting the lowercase entirely here and adding it to the client repo just before the string matching. @nicolasnoble any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

From a clean coding perspective, this is a proper change.
The issue that prompted this small PR tho is that we are lowercasing user input before it is sent in the request. The user needs their input casing preserved so we still need to ensure that is preserved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if I understood your point. Could you clarify the suggestion/point?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am just saying that it is wise to move the modification of the data as close to where you need the data changed.
Basically, i agree with you.

ToLowerCase(type); // to string-match the type

cb::RequestParameter param;
param.name = name;
Expand Down
Loading