Add Feature(build): auto-generate Modelfile and add --regenerate flag#414
Add Feature(build): auto-generate Modelfile and add --regenerate flag#414gmarav05 wants to merge 1 commit intomodelpack:mainfrom
Conversation
Summary of ChangesHello @gmarav05, 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 significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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 introduces a helpful feature to automatically generate a Modelfile during the build process if one is not found, along with a --regenerate flag to force this action. The implementation is mostly solid, but I've identified a few areas for improvement regarding path resolution, error handling, and code clarity in the runBuild function. My suggestions aim to make the new logic more robust and easier to understand. Addressing these points will help prevent potential bugs and improve the maintainability of the code.
| modelfilePath := buildConfig.Modelfile | ||
| if modelfilePath == "" { | ||
| modelfilePath = filepath.Join(workDir, configmodelfile.DefaultModelfileName) | ||
| } |
There was a problem hiding this comment.
The modelfilePath is not correctly resolved against the workDir. When a relative path is used for the modelfile (which is the default), os.Stat(modelfilePath) will check for the file in the current working directory of the modctl process, not in the specified workDir. This can lead to incorrect behavior, such as failing to find an existing Modelfile or generating a new one in the wrong location. The path should be explicitly joined with workDir if it's relative.
| modelfilePath := buildConfig.Modelfile | |
| if modelfilePath == "" { | |
| modelfilePath = filepath.Join(workDir, configmodelfile.DefaultModelfileName) | |
| } | |
| modelfilePath := buildConfig.Modelfile | |
| if modelfilePath == "" { | |
| modelfilePath = filepath.Join(workDir, configmodelfile.DefaultModelfileName) | |
| } else if !filepath.IsAbs(modelfilePath) { | |
| modelfilePath = filepath.Join(workDir, modelfilePath) | |
| } |
cmd/build.go
Outdated
| shouldGenerate := false | ||
|
|
||
| if buildConfig.Regenerate { | ||
| fmt.Println("Regenerate flag detected. Regenerating Modelfile...") | ||
| shouldGenerate = true | ||
| } else if _, err := os.Stat(modelfilePath); os.IsNotExist(err) { | ||
| fmt.Println("No Modelfile found. Generating automatically...") | ||
| shouldGenerate = true | ||
| } |
There was a problem hiding this comment.
The error from os.Stat is only checked for os.IsNotExist. Any other error (e.g., permission denied) will be silently ignored. This will cause shouldGenerate to be false, and the build will proceed, likely failing later when trying to read the modelfile. It's better to handle other potential errors from os.Stat explicitly for robustness.
| shouldGenerate := false | |
| if buildConfig.Regenerate { | |
| fmt.Println("Regenerate flag detected. Regenerating Modelfile...") | |
| shouldGenerate = true | |
| } else if _, err := os.Stat(modelfilePath); os.IsNotExist(err) { | |
| fmt.Println("No Modelfile found. Generating automatically...") | |
| shouldGenerate = true | |
| } | |
| shouldGenerate := buildConfig.Regenerate | |
| if shouldGenerate { | |
| fmt.Println("Regenerate flag detected. Regenerating Modelfile...") | |
| } else { | |
| _, err := os.Stat(modelfilePath) | |
| if os.IsNotExist(err) { | |
| fmt.Println("No Modelfile found. Generating automatically...") | |
| shouldGenerate = true | |
| } else if err != nil { | |
| return fmt.Errorf("error checking for Modelfile at %s: %w", modelfilePath, err) | |
| } | |
| } |
| genConfig := configmodelfile.NewGenerateConfig() | ||
| genConfig.Workspace = workDir | ||
| genConfig.Output = workDir | ||
| genConfig.Overwrite = true | ||
|
|
||
| if err := genConfig.Convert(workDir); err != nil { | ||
| return fmt.Errorf("failed to prepare modelfile generation: %w", err) | ||
| } |
There was a problem hiding this comment.
The setup for genConfig is a bit confusing. genConfig.Output is set and then converted to an absolute path inside genConfig.Convert, but this value is never used. The only relevant side effect of genConfig.Convert seems to be making genConfig.Workspace an absolute path. This can be simplified to make the intent clearer and reduce reliance on the side effects of Convert.
| genConfig := configmodelfile.NewGenerateConfig() | |
| genConfig.Workspace = workDir | |
| genConfig.Output = workDir | |
| genConfig.Overwrite = true | |
| if err := genConfig.Convert(workDir); err != nil { | |
| return fmt.Errorf("failed to prepare modelfile generation: %w", err) | |
| } | |
| genConfig := configmodelfile.NewGenerateConfig() | |
| absWorkDir, err := filepath.Abs(workDir) | |
| if err != nil { | |
| return fmt.Errorf("failed to resolve workspace path: %w", err) | |
| } | |
| genConfig.Workspace = absWorkDir | |
| genConfig.Overwrite = true |
Signed-off-by: Aravind <gmarav005@gmail.com>
gmarav05
force-pushed
the
feature/auto-generate-modelfile
branch
from
e093354 to
8d8dc4b
Compare
1 participant
This PR simplifies the modctl build workflow.
Now, if a Modelfile is missing, it will be generated automatically during build.
A new --regenerate flag is added to force regeneration before building.
Fixes #362
What changed
build auto-generates Modelfile if not found
Added --regenerate flag
Existing behavior will be unchanged if a Modelfile already exists
Tested