Skip to content

Commit

Permalink
#154 hotfix saved config paths are not multiplatform (#157)
Browse files Browse the repository at this point in the history
* Hotfix path gen

* Hotfix path gen, check for escape characters

* Added tests for new function

* Removed unecessary cases

* Removed unecessary cases and updated tests

* Adding fix

* Adding fix

* Removed unrelated changes

* Removed unrelated changes

* updating test

* updating test

* test push

* test push

* Push fix Different os returning different results

* use filepath instead of path

* fixed path uniformize method

* fixed error display

---------

Co-authored-by: Rayan MARMAR <[email protected]>
  • Loading branch information
OmarElChamaa and RayanMarmar authored May 15, 2024
1 parent 7dcc9c7 commit bd61a14
Show file tree
Hide file tree
Showing 11 changed files with 83 additions and 62 deletions.
1 change: 1 addition & 0 deletions cmd/model/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ func init() {
func runAdd(cmd *cobra.Command, args []string) {
err := addController.Run(args, customArgs)
if err != nil {
app.UI().Error().Println(err.Error())
os.Exit(1)
}
}
25 changes: 12 additions & 13 deletions internal/config/models_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"github.com/easy-model-fusion/emf-cli/test/mock"
"gopkg.in/yaml.v3"
"os"
"path"
"path/filepath"
"testing"

Expand Down Expand Up @@ -579,9 +578,9 @@ func TestValidate_DownloadedAndBinaryFalse_ConfirmFalse(t *testing.T) {
test.AssertEqual(t, err, nil, "Error while loading configuration file.")

// Create a temporary directory representing the model base path
modelName := path.Join("microsoft", "phi-2")
modelDirectory := path.Join(app.DownloadDirectoryPath, modelName)
modelPath := path.Join(modelDirectory, "model")
modelName := filepath.Join("microsoft", "phi-2")
modelDirectory := filepath.Join(app.DownloadDirectoryPath, modelName)
modelPath := filepath.Join(modelDirectory, "model")
err = os.MkdirAll(modelPath, 0750)
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -617,9 +616,9 @@ func TestValidate_DownloadedAndBinaryFalse_ConfirmTrueAndRemove(t *testing.T) {
test.AssertEqual(t, err, nil, "Error while loading configuration file.")

// Create a temporary directory representing the model base path
modelName := path.Join("microsoft", "phi-2")
modelDirectory := path.Join(app.DownloadDirectoryPath, modelName)
modelPath := path.Join(modelDirectory, "model")
modelName := filepath.Join("microsoft", "phi-2")
modelDirectory := filepath.Join(app.DownloadDirectoryPath, modelName)
modelPath := filepath.Join(modelDirectory, "model")
err = os.MkdirAll(modelPath, 0750)
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -660,9 +659,9 @@ func TestValidate_Downloaded_ConfirmFalse(t *testing.T) {
test.AssertEqual(t, err, nil, "Error while loading configuration file.")

// Create a temporary directory representing the model base path
modelName := path.Join("microsoft", "phi-2")
modelDirectory := path.Join(app.DownloadDirectoryPath, modelName)
modelPath := path.Join(modelDirectory, "model")
modelName := filepath.Join("microsoft", "phi-2")
modelDirectory := filepath.Join(app.DownloadDirectoryPath, modelName)
modelPath := filepath.Join(modelDirectory, "model")
err = os.MkdirAll(modelPath, 0750)
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -698,9 +697,9 @@ func TestValidate_Downloaded_ConfirmTrue(t *testing.T) {
test.AssertEqual(t, err, nil, "Error while loading configuration file.")

// Create a temporary directory representing the model base path
modelName := path.Join("microsoft", "phi-2")
modelDirectory := path.Join(app.DownloadDirectoryPath, modelName)
modelPath := path.Join(modelDirectory, "model")
modelName := filepath.Join("microsoft", "phi-2")
modelDirectory := filepath.Join(app.DownloadDirectoryPath, modelName)
modelPath := filepath.Join(modelDirectory, "model")
err = os.MkdirAll(modelPath, 0750)
if err != nil {
t.Fatal(err)
Expand Down
4 changes: 2 additions & 2 deletions internal/controller/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
"os"
"os/exec"
"os/signal"
"path"
"path/filepath"
"strings"
"syscall"
"time"
Expand Down Expand Up @@ -193,7 +193,7 @@ func (bc BuildController) Build(libraryPath string) (err error) {
func (bc BuildController) createModelsSymbolicLink() error {
// Create symbolic link to models
modelsPath := "models"
distPath := path.Join(bc.DestinationDir, "models")
distPath := filepath.Join(bc.DestinationDir, "models")

app.UI().Info().Println(fmt.Sprintf("Creating symbolic link from %s to %s", modelsPath, distPath))

Expand Down
1 change: 0 additions & 1 deletion internal/controller/model/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ func (ac AddController) Run(args []string, customArgs downloadermodel.Args) erro
var result resultutil.ExecutionResult
warnings, err := ac.processAdd(selectedModel, customArgs)
result.AddWarnings(warnings)
result.SetError(err)
result.Display("Operation succeeded", "Operation failed")

return err
Expand Down
3 changes: 2 additions & 1 deletion internal/model/generation.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package model
import (
"fmt"
"github.com/easy-model-fusion/emf-cli/internal/codegen"
"github.com/easy-model-fusion/emf-cli/internal/utils/stringutil"
"github.com/easy-model-fusion/emf-cli/pkg/huggingface"
"golang.org/x/text/cases"
"golang.org/x/text/language"
Expand Down Expand Up @@ -87,7 +88,7 @@ func (m *Model) GenFile() *codegen.File {
// GenModelPath returns the model path to be used in the code generation
func (m *Model) GenModelPath() string {
if m.IsDownloaded {
return m.Path
return stringutil.PathUniformize(m.Path)
}
return m.Name
}
Expand Down
8 changes: 4 additions & 4 deletions internal/model/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
"github.com/easy-model-fusion/emf-cli/internal/utils/dotenv"
"github.com/easy-model-fusion/emf-cli/internal/utils/stringutil"
"github.com/easy-model-fusion/emf-cli/pkg/huggingface"
"path"
"path/filepath"
"strings"
)

Expand Down Expand Up @@ -194,7 +194,7 @@ func (m Models) FilterWithAddToBinaryFileTrue() Models {

// GetBasePath return the base path to the model
func (m *Model) GetBasePath() string {
return path.Join(app.DownloadDirectoryPath, m.Name)
return filepath.Join(app.DownloadDirectoryPath, m.Name)
}

// UpdatePaths to update the model's path to elements accordingly to its configuration.
Expand All @@ -203,9 +203,9 @@ func (m *Model) UpdatePaths() {
basePath := m.GetBasePath()
modelPath := basePath
if m.Module == huggingface.TRANSFORMERS {
modelPath = path.Join(modelPath, "model")
modelPath = filepath.Join(modelPath, "model")
for i, tokenizer := range m.Tokenizers {
m.Tokenizers[i].Path = path.Join(basePath, tokenizer.Class)
m.Tokenizers[i].Path = filepath.Join(basePath, tokenizer.Class)
}
}
m.Path = modelPath
Expand Down
10 changes: 5 additions & 5 deletions internal/model/model_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
"github.com/easy-model-fusion/emf-cli/internal/app"
"github.com/easy-model-fusion/emf-cli/pkg/huggingface"
"github.com/easy-model-fusion/emf-cli/test"
"path"
"path/filepath"
"testing"
)

Expand Down Expand Up @@ -289,7 +289,7 @@ func TestGetBasePath(t *testing.T) {
basePath := model.GetBasePath()

// Assert
test.AssertEqual(t, basePath, path.Join(app.DownloadDirectoryPath, model.Name))
test.AssertEqual(t, basePath, filepath.Join(app.DownloadDirectoryPath, model.Name))
}

// TestUpdatePaths_Default tests the Model.UpdatePaths for a default model.
Expand All @@ -301,7 +301,7 @@ func TestUpdatePaths_Default(t *testing.T) {
model.UpdatePaths()

// Assert
test.AssertEqual(t, model.Path, path.Join(app.DownloadDirectoryPath, model.Name))
test.AssertEqual(t, model.Path, filepath.Join(app.DownloadDirectoryPath, model.Name))
}

// TestUpdatePaths_Transformers tests the Model.UpdatePaths for a transformers model.
Expand All @@ -314,7 +314,7 @@ func TestUpdatePaths_Transformers(t *testing.T) {
model.UpdatePaths()

// Assert
test.AssertEqual(t, model.Path, path.Join(app.DownloadDirectoryPath, model.Name, "model"))
test.AssertEqual(t, model.Path, filepath.Join(app.DownloadDirectoryPath, model.Name, "model"))
}

// TestUpdatePaths_TransformersTokenizers tests the Model.UpdatePaths for a transformers model.
Expand All @@ -328,7 +328,7 @@ func TestUpdatePaths_TransformersTokenizers(t *testing.T) {
model.UpdatePaths()

// Assert
test.AssertEqual(t, model.Tokenizers[0].Path, path.Join(app.DownloadDirectoryPath, model.Name, "tokenizer"))
test.AssertEqual(t, model.Tokenizers[0].Path, filepath.Join(app.DownloadDirectoryPath, model.Name, "tokenizer"))
}

// TestFilterWithClass_Success tests the Tokenizers.FilterWithClass function to return the correct models.
Expand Down
11 changes: 5 additions & 6 deletions internal/model/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"github.com/easy-model-fusion/emf-cli/internal/utils/stringutil"
"github.com/easy-model-fusion/emf-cli/pkg/huggingface"
"os"
"path"
"path/filepath"
"strings"
)
Expand Down Expand Up @@ -118,7 +117,7 @@ func BuildModelsFromDevice(accessToken string) Models {
}

// Get all the models for the provider
providerPath := path.Join(app.DownloadDirectoryPath, provider.Name())
providerPath := filepath.Join(app.DownloadDirectoryPath, provider.Name())
providerModels, err := os.ReadDir(providerPath)
if err != nil {
continue
Expand All @@ -133,8 +132,8 @@ func BuildModelsFromDevice(accessToken string) Models {
}

// Model info
modelName := path.Join(provider.Name(), providerModel.Name())
modelPath := path.Join(providerPath, providerModel.Name())
modelName := filepath.Join(provider.Name(), providerModel.Name())
modelPath := filepath.Join(providerPath, providerModel.Name())

// Fetching model from huggingface
huggingfaceModel, err := app.H().GetModelById(modelName, accessToken)
Expand Down Expand Up @@ -181,15 +180,15 @@ func BuildModelsFromDevice(accessToken string) Models {

// Model folder exists : meaning the model is downloaded
if directory.Name() == "model" {
modelMapped.Path = path.Join(modelPath, "model")
modelMapped.Path = filepath.Join(modelPath, "model")
modelMapped.AddToBinaryFile = true
modelMapped.IsDownloaded = true
continue
}

// Otherwise : directory is considered as a tokenizer
tokenizer := Tokenizer{
Path: path.Join(modelPath, directory.Name()),
Path: filepath.Join(modelPath, directory.Name()),
Class: directory.Name(),
}
modelMapped.Tokenizers = append(modelMapped.Tokenizers, tokenizer)
Expand Down
23 changes: 11 additions & 12 deletions internal/model/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"github.com/easy-model-fusion/emf-cli/test/mock"
"github.com/pterm/pterm"
"os"
"path"
"path/filepath"
"testing"

Expand Down Expand Up @@ -83,9 +82,9 @@ func TestDownloadedOnDevice_True(t *testing.T) {
// TestModelDownloadedOnDevice_UseBasePath_True tests the ModelDownloadedOnDevice function to return true.
func TestModelDownloadedOnDevice_UseBasePath_True(t *testing.T) {
// Create a temporary directory representing the model base path
modelName := path.Join("microsoft", "phi-2")
modelDirectory := path.Join(app.DownloadDirectoryPath, modelName)
modelPath := path.Join(modelDirectory, "model")
modelName := filepath.Join("microsoft", "phi-2")
modelDirectory := filepath.Join(app.DownloadDirectoryPath, modelName)
modelPath := filepath.Join(modelDirectory, "model")
err := os.MkdirAll(modelPath, 0750)
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -243,7 +242,7 @@ func TestGetTokenizersNotDownloadedOnDevice_NotMissing(t *testing.T) {
// TestBuildModelsFromDevice_Custom tests the BuildModelsFromDevice function to work for custom configured models.
func TestBuildModelsFromDevice_Custom(t *testing.T) {
// Create a temporary directory representing the path to the custom model
modelPath := path.Join(app.DownloadDirectoryPath, "custom-provider", "custom-model")
modelPath := filepath.Join(app.DownloadDirectoryPath, "custom-provider", "custom-model")
modelPath = filepath.ToSlash(modelPath)
err := os.MkdirAll(modelPath, 0750)
if err != nil {
Expand All @@ -267,7 +266,7 @@ func TestBuildModelsFromDevice_Custom(t *testing.T) {
// TestBuildModelsFromDevice_HuggingfaceEmpty tests the BuildModelsFromDevice function to work for huggingface empty models.
func TestBuildModelsFromDevice_HuggingfaceEmpty(t *testing.T) {
// Create a temporary directory representing the path to the model which is empty
modelDirectoryPath := path.Join(app.DownloadDirectoryPath, "stabilityai", "sdxl-turbo")
modelDirectoryPath := filepath.Join(app.DownloadDirectoryPath, "stabilityai", "sdxl-turbo")
err := os.MkdirAll(modelDirectoryPath, 0750)
if err != nil {
t.Fatal(err)
Expand All @@ -285,9 +284,9 @@ func TestBuildModelsFromDevice_HuggingfaceEmpty(t *testing.T) {
// TestBuildModelsFromDevice_HuggingfaceDiffusers tests the BuildModelsFromDevice function to work for huggingface diffusers models.
func TestBuildModelsFromDevice_HuggingfaceDiffusers(t *testing.T) {
// Create a temporary directory representing the path to the diffusers model which is not empty
modelName := path.Join("stabilityai", "sdxl-turbo")
modelDirectory := path.Join(app.DownloadDirectoryPath, modelName)
err := os.MkdirAll(path.Join(modelDirectory, "not-empty"), 0750)
modelName := filepath.Join("stabilityai", "sdxl-turbo")
modelDirectory := filepath.Join(app.DownloadDirectoryPath, modelName)
err := os.MkdirAll(filepath.Join(modelDirectory, "not-empty"), 0750)
if err != nil {
t.Fatal(err)
}
Expand All @@ -311,9 +310,9 @@ func TestBuildModelsFromDevice_HuggingfaceDiffusers(t *testing.T) {
// TestBuildModelsFromDevice_HuggingfaceTransformers tests the BuildModelsFromDevice function to work for huggingface transformers models.
func TestBuildModelsFromDevice_HuggingfaceTransformers(t *testing.T) {
// Create a temporary directory representing the path to the transformers model
modelName := path.Join("microsoft", "phi-2")
modelDirectory := path.Join(app.DownloadDirectoryPath, modelName)
modelPath := path.Join(modelDirectory, "model")
modelName := filepath.Join("microsoft", "phi-2")
modelDirectory := filepath.Join(app.DownloadDirectoryPath, modelName)
modelPath := filepath.Join(modelDirectory, "model")
err := os.MkdirAll(modelPath, 0750)
if err != nil {
t.Fatal(err)
Expand Down
25 changes: 19 additions & 6 deletions internal/utils/stringutil/string.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,30 @@ func SplitPath(path string) []string {
return elements
}

// PathUniformize returns uniformized path regarding the device OS.
func PathUniformize(path string) string {
// Replace backslashes with forward slashes
path = strings.ReplaceAll(path, "\\", "/")
// PathRemoveSpecialCharacter adds a slash before a special character
func PathRemoveSpecialCharacter(path string) string {
//List of special characters that need to be escaped
specialChars := map[string]string{
"\\": "/",
}

// Resolve dots and double slashes
path = filepath.Clean(path)
// Replace special characters with their escaped form
for oldChar, newChar := range specialChars {
if strings.Contains(path, oldChar) {
path = strings.ReplaceAll(path, oldChar, newChar)
}
}

return path
}

// PathUniformize returns uniformized path regarding the device OS.
func PathUniformize(path string) string {
path = filepath.Clean(path)
// Replace backslashes with forward slashes
return filepath.ToSlash(path)
}

// ParseOptions parses a string containing options in various formats
// Returns a slice of strings where each string represents an option.
func ParseOptions(input string) []string {
Expand Down
34 changes: 22 additions & 12 deletions internal/utils/stringutil/string_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package stringutil

import (
"github.com/easy-model-fusion/emf-cli/test"
"path/filepath"
"testing"
)

Expand Down Expand Up @@ -48,24 +47,24 @@ func TestPathUniformize_Success(t *testing.T) {
input string
expected string
}{
{"C:\\path\\to\\file", "C:/path/to/file"},
{"C:\\path\\to\\..\\file", "C:/path/file"},
{"C:\\path\\to\\dir\\..\\file", "C:/path/to/file"},
{"C:\\path\\with\\double\\\\slashes", "C:/path/with/double/slashes"},
{"C:\\path\\with\\dots\\..", "C:/path/with"},
{"C:\\path\\with\\dots\\..\\..", "C:/path"},
{"C:\\path\\with\\dots\\.", "C:/path/with/dots"},
{"C:\\path\\with\\dots\\.\\.", "C:/path/with/dots"},
{"C:\\path\\with\\dots\\.\\.\\..", "C:/path/with"},
{"C:\\path\\with\\dots\\.\\.\\..\\file", "C:/path/with/file"},
{"C:/path/to/file", "C:/path/to/file"},
{"C:/path/to/../file", "C:/path/file"},
{"C:/path/to/dir/../file", "C:/path/to/file"},
{"C:/path/with/double/slashes", "C:/path/with/double/slashes"},
{"C:/path/with/dots/..", "C:/path/with"},
{"C:/path/with/dots/../..", "C:/path"},
{"C:/path/with/dots/.", "C:/path/with/dots"},
{"C:/path/with/dots/./.", "C:/path/with/dots"},
{"C:/path/with/dots/././..", "C:/path/with"},
{"C:/path/with/dots/././../file", "C:/path/with/file"},
}

for _, item := range items {
// Execute
result := PathUniformize(item.input)

// Assert
test.AssertEqual(t, result, filepath.Clean(item.expected))
test.AssertEqual(t, result, item.expected)
}
}

Expand Down Expand Up @@ -162,3 +161,14 @@ func TestOptionsMapToSlice(t *testing.T) {
test.AssertEqual(t, SliceContainsItem(optionsSlice, "key3=module.value"), true)
test.AssertEqual(t, SliceContainsItem(optionsSlice, "key4='text'"), true)
}

func TestPathRemoveSpecialCharacter(t *testing.T) {
// Init
testPath := "models\\FredZhang7\\anime-anything-promptgen-v2\\model"

// Execute
updatedPath := PathRemoveSpecialCharacter(testPath)
expectedPath := "models/FredZhang7/anime-anything-promptgen-v2/model"

test.AssertEqual(t, updatedPath, expectedPath)
}

0 comments on commit bd61a14

Please sign in to comment.