fix: handle spaces in file paths in Modelfile parsing #293
Conversation
Summary of ChangesHello @raravena80, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a crucial fix to the Modelfile parsing mechanism, specifically addressing an error that occurred when file paths containing unquoted spaces were used. By refining the argument parsing logic, the system can now correctly interpret such paths, significantly improving the robustness and user-friendliness of Modelfiles, especially those that are auto-generated. The changes maintain full backward compatibility and are thoroughly validated with updated test suites. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue with parsing Modelfile paths that contain spaces by joining arguments in parseStringArgs. The changes look good and the accompanying test updates are appropriate. I've added a couple of suggestions to further improve the robustness of the argument parsing. One suggestion is to enhance the validation to reject arguments that consist solely of whitespace. The other is to add more test cases to cover these whitespace-related edge cases. Overall, this is a solid fix.
pkg/modelfile/parser/args_parser.go
Outdated
| if joined == "" { | ||
| return nil, errors.New("empty args") | ||
| } |
There was a problem hiding this comment.
The current check if joined == "" only handles cases where the argument is an empty string. It doesn't handle arguments that consist only of whitespace, such as " " or "" "". This could lead to creating nodes with paths that are just whitespace, which is likely undesirable.
To make the validation more robust, I suggest checking if the trimmed version of the joined string is empty. This will correctly reject arguments that are composed entirely of whitespace, while preserving any intentional whitespace in valid (quoted) arguments.
| if joined == "" { | |
| return nil, errors.New("empty args") | |
| } | |
| if strings.TrimSpace(joined) == "" { | |
| return nil, errors.New("empty args") | |
| } |
| {[]string{"example", "workflows_Wan2.1/image_to_video_wan_480p_example.json"}, 13, 14, false, "example workflows_Wan2.1/image_to_video_wan_480p_example.json"}, | ||
| } |
There was a problem hiding this comment.
To improve test coverage and ensure correct handling of various whitespace scenarios, it would be beneficial to add a few more test cases. Specifically, we should test for:
- Arguments that consist only of whitespace (e.g.,
" "). - Multiple empty string arguments (e.g.,
"" ""). - Arguments with leading or trailing spaces that should be preserved.
These tests will help verify that whitespace-only arguments are rejected while intended whitespace in valid paths is preserved.
{[]string{"example", "workflows_Wan2.1/image_to_video_wan_480p_example.json"}, 13, 14, false, "example workflows_Wan2.1/image_to_video_wan_480p_example.json"},
// Test cases for whitespace handling
{[]string{" "}, 15, 16, true, ""},
{[]string{"", ""}, 17, 18, true, ""},
{[]string{" a "}, 19, 20, false, " a "},
}
Fixes issue #207 where Modelfile parsing would fail with 'invalid args' error when file paths contained spaces without quotes. Changes: - Modified parseStringArgs() to join multiple arguments with spaces - This handles unquoted paths like 'CONFIG example workflows/file.json' - Maintains backward compatibility with quoted paths - Updated tests to reflect new behavior - Added comprehensive test coverage for edge cases The fix allows both: - CONFIG "path with spaces/file.json" (quoted - existing behavior) - CONFIG path with spaces/file.json (unquoted - new behavior) Signed-off-by: Ricardo Aravena <[email protected]>
…verage Address feedback from @gemini-code-assist[bot] review: 1. Enhanced validation in parseStringArgs(): - Use strings.TrimSpace() to reject arguments consisting solely of whitespace - Prevents creating nodes with whitespace-only paths - Maintains preservation of intentional whitespace in valid arguments 2. Added comprehensive test cases for whitespace handling: - Whitespace-only arguments (should be rejected) - Multiple empty string arguments (should be rejected) - Arguments with leading/trailing spaces (should be preserved) - Mixed whitespace scenarios This improves robustness while maintaining backward compatibility and the core functionality of handling unquoted paths with spaces. Signed-off-by: Ricardo Aravena <[email protected]>
raravena80
force-pushed
the
fix-spaces-in-modelfile-paths
branch
from
697b901 to
2f68d85
Compare
|
@raravena80 Thanks for your work! The original design is the value only includes one element, e.g, |
|
@chlins that sounds like a new requirement. Do we have the multiple line requirement per config value defined in the spec? Also, sounds like we need a different set of test cases for that. As long as we have the multiple line requirement well document imo it should be ok, we can't prevent users to put two different strings in a CONFIG line thinking that they are multiple different values. Let me know what you think. |
@raravena80 So far, we have not received such requirement, and it seems that there is no such definition in the spec, because the modelfile is only a concept specific to the modctl tool itself and not a specification-defined term. |
2 participants
Summary
Fixes #207 - Resolves the issue where Modelfile parsing would fail with 'invalid args' error when file paths contained spaces without quotes.
Problem
When using auto-generated Modelfiles with file paths containing spaces (like
CONFIG example workflows_Wan2.1/image_to_video_wan_480p_example.json), the parser would fail with:Solution
Modified the
parseStringArgs()function inpkg/modelfile/parser/args_parser.goto:CONFIG path with spaces/file.jsonChanges Made
parseStringArgs()to usestrings.Join(args, " ")for multiple argumentsTesting
All tests pass:
go test ./pkg/modelfile/parser/go test ./pkg/modelfile/Backward Compatibility:
CONFIG "path with spaces/file.json"CONFIG regular_file.jsonExample Usage
Before (would fail):
CONFIG example workflows_Wan2.1/image_to_video_wan_480p_example.json # Error: parse command line error on line X: invalid argsAfter (now works):
This makes auto-generated Modelfiles more robust and user-friendly while maintaining full backward compatibility.