modellist: fix removal and improve locking

There were issues with removing clones or models with clones from the
model page, as well as trying to remove a model that no longer exists on
disk, despite being in settings. These should now be resolved.

Signed-off-by: Jared Van Bortel <jared@nomic.ai>

gpt4all-chat/qml/ModelSettings.qml

@@ -92,7 +92,7 @@ MySettingsTab {
                 enabled: root.currentModelInfo.isClone
                 text: qsTr("Remove")
                 onClicked: {
-                    ModelList.removeClone(root.currentModelInfo);
+                    ModelList.uninstall(root.currentModelInfo);
                     comboBox.currentIndex = 0;
                 }
             }

gpt4all-chat/src/download.cpp

@@ -330,36 +330,27 @@ void Download::installCompatibleModel(const QString &modelName, const QString &a
 
 void Download::removeModel(const QString &modelFile)
 {
-    const QString filePath = MySettings::globalInstance()->modelPath() + modelFile;
+    auto *modelList = ModelList::globalInstance();
-    QFile incompleteFile(ModelList::globalInstance()->incompleteDownloadPath(modelFile));
+    auto *mySettings = MySettings::globalInstance();
-    if (incompleteFile.exists()) {
+
+    QFile incompleteFile(modelList->incompleteDownloadPath(modelFile));
+    if (incompleteFile.exists())
         incompleteFile.remove();
-    }
 
     bool shouldRemoveInstalled = false;
-    QFile file(filePath);
+
+    QFile file(mySettings->modelPath() + modelFile);
+    const ModelInfo info = modelList->modelInfoByFilename(modelFile);
+
+    Network::globalInstance()->trackEvent("remove_model", { {"model", modelFile}, {"exists", file.exists()} });
+
     if (file.exists()) {
-        const ModelInfo info = ModelList::globalInstance()->modelInfoByFilename(modelFile);
+        modelList->uninstall(info);
-        MySettings::globalInstance()->eraseModel(info.id());
+        if (!info.isClone())
-        shouldRemoveInstalled = info.installed && !info.isClone() && (info.isDiscovered() || info.isCompatibleApi || info.description() == "" /*indicates sideloaded*/);
+            file.remove();
-        if (shouldRemoveInstalled)
-            ModelList::globalInstance()->removeInstalled(info);
-        Network::globalInstance()->trackEvent("remove_model", { {"model", modelFile} });
-        file.remove();
-        emit toastMessage(tr("Model \"%1\" is removed.").arg(info.name()));
     }
 
-    if (!shouldRemoveInstalled) {
+    emit toastMessage(tr("Model \"%1\" is removed.").arg(info.name()));
-        QVector<QPair<int, QVariant>> data {
-            { ModelList::InstalledRole, false },
-            { ModelList::BytesReceivedRole, 0 },
-            { ModelList::BytesTotalRole, 0 },
-            { ModelList::TimestampRole, 0 },
-            { ModelList::SpeedRole, QString() },
-            { ModelList::DownloadErrorRole, QString() },
-        };
-        ModelList::globalInstance()->updateDataByFilename(modelFile, data);
-    }
 }
 
 void Download::handleSslErrors(QNetworkReply *reply, const QList<QSslError> &errors)

gpt4all-chat/src/modellist.cpp

@@ -1041,21 +1041,21 @@ void ModelList::updateDataByFilename(const QString &filename, QVector<QPair<int,
     if (data.isEmpty())
         return; // no-op
 
-    QVector<QString> modelsById;
     {
         QMutexLocker locker(&m_mutex);
+        QStringList modelsById;
         for (auto *info : std::as_const(m_models))
             if (info->filename() == filename)
                 modelsById.append(info->id());
-    }
 
-    if (modelsById.isEmpty()) {
+        if (modelsById.isEmpty()) {
-        qWarning() << "ERROR: cannot update model as list does not contain file" << filename;
+            qWarning() << "ERROR: cannot update model as list does not contain file" << filename;
-        return;
+            return;
-    }
+        }
 
-    for (auto &id : std::as_const(modelsById))
+        for (auto &id : std::as_const(modelsById))
-        updateData(id, data);
+            updateDataInternal(id, data, locker, /*relock*/ &id != &modelsById.constLast());
+    }
 }
 
 ModelInfo ModelList::modelInfo(const QString &id) const
@@ -1118,53 +1118,84 @@ QString ModelList::clone(const ModelInfo &model)
     return id;
 }
 
-void ModelList::removeClone(const ModelInfo &model)
+void ModelList::uninstall(const ModelInfo &model)
 {
     Q_ASSERT(QThread::currentThread() == thread());
-    Q_ASSERT(model.isClone());
-    if (!model.isClone())
-        return;
 
-    removeInternal(model);
+    {
-    emit layoutChanged();
+        QMutexLocker lock(&m_mutex);
+
+        if (!model.isClone()) {
+            QStringList modelsById;
+            auto filename = model.filename();
+            for (const auto *info : std::as_const(m_models))
+                if (info->filename() == filename && info->isClone())
+                    modelsById << info->id();
+
+            for (const auto &id : std::as_const(modelsById))
+                removeInternal(id, lock);
+        }
+
+        auto id = model.id();
+        if (model.isClone() || model.isDiscovered() || model.isCompatibleApi
+            || model.description() == "" /*sideloaded*/
+        ) {
+            // model can be completely removed from list
+            removeInternal(id, lock, /*relock*/ false);
+            emit selectableModelListChanged();
+        } else {
+            // Model came from models.json and needs to be reset instead of removed
+            QVector<QPair<int, QVariant>> data {
+                { ModelList::InstalledRole,     false     },
+                { ModelList::BytesReceivedRole, 0         },
+                { ModelList::BytesTotalRole,    0         },
+                { ModelList::TimestampRole,     0         },
+                { ModelList::SpeedRole,         QString() },
+                { ModelList::DownloadErrorRole, QString() },
+            };
+            updateDataInternal(id, data, lock, /*relock*/ false);
+
+            // erase settings
+            MySettings::globalInstance()->eraseModel(id);
+        }
+    }
 }
 
-void ModelList::removeInstalled(const ModelInfo &model)
+void ModelList::removeInternal(const QString &id, QMutexLocker<QMutex> &lock, bool relock)
 {
     Q_ASSERT(QThread::currentThread() == thread());
-    Q_ASSERT(model.installed);
+    Q_ASSERT(lock.isLocked());
-    Q_ASSERT(!model.isClone());
-    Q_ASSERT(model.isDiscovered() || model.isCompatibleApi || model.description() == "" /*indicates sideloaded*/);
-    removeInternal(model);
-    emit layoutChanged();
-}
 
-void ModelList::removeInternal(const ModelInfo &model)
+    bool hasModel = m_modelMap.contains(id);
-{
-    Q_ASSERT(QThread::currentThread() == thread());
-    const bool hasModel = contains(model.id());
     Q_ASSERT(hasModel);
     if (!hasModel) {
-        qWarning() << "ERROR: model list does not contain" << model.id();
+        qWarning() << "ERROR: model list does not contain" << id;
         return;
     }
 
-    int indexOfModel = 0;
+    auto mapIt = std::as_const(m_modelMap).find(id);
-    {
+    qsizetype listIdx = m_models.indexOf(*mapIt);
-        QMutexLocker locker(&m_mutex);
+
-        ModelInfo *info = m_modelMap.value(model.id());
+    lock.unlock();
-        indexOfModel = m_models.indexOf(info);
+    beginRemoveRows({}, listIdx, listIdx);
-    }
+    lock.relock();
-    beginRemoveRows(QModelIndex(), indexOfModel, indexOfModel);
+
-    {
+    // remove entry
-        QMutexLocker locker(&m_mutex);
+    auto *info = *mapIt;
-        ModelInfo *info = m_models.takeAt(indexOfModel);
+    Q_ASSERT(std::as_const(m_modelMap).find(id) == mapIt);
-        m_modelMap.remove(info->id());
+    Q_ASSERT(m_models.indexOf(info) == listIdx);
-        delete info;
+    m_modelMap.erase(mapIt);
-    }
+    m_models.remove(listIdx);
+    delete info;
+
+    lock.unlock();
     endRemoveRows();
-    emit selectableModelListChanged();
+
-    MySettings::globalInstance()->eraseModel(model.id());
+    // erase settings
+    MySettings::globalInstance()->eraseModel(id);
+
+    if (relock)
+        lock.relock();
 }
 
 QString ModelList::uniqueModelName(const ModelInfo &model) const
@@ -1927,15 +1958,14 @@ void ModelList::setDiscoverSort(DiscoverSort sort)
 void ModelList::clearDiscoveredModels()
 {
     // NOTE: This could be made much more efficient
-    QList<ModelInfo> infos;
+    QMutexLocker locker(&m_mutex);
-    {
+    QStringList ids;
-        QMutexLocker locker(&m_mutex);
+    for (auto *info : std::as_const(m_models))
-        for (auto *info : std::as_const(m_models))
+        if (info->isDiscovered() && !info->installed)
-            if (info->isDiscovered() && !info->installed)
+            ids << info->id();
-                infos.append(*info);
+
-    }
+    for (auto &id : std::as_const(ids))
-    for (auto &info : std::as_const(infos))
+        removeInternal(id, locker, /*relock*/ &id != &ids.constLast());
-        removeInternal(info);
 }
 
 float ModelList::discoverProgress() const

gpt4all-chat/src/modellist.h

@@ -421,8 +421,8 @@ public:
     Q_INVOKABLE ModelInfo modelInfoByFilename(const QString &filename) const;
     Q_INVOKABLE bool isUniqueName(const QString &name) const;
     Q_INVOKABLE QString clone(const ModelInfo &model);
-    Q_INVOKABLE void removeClone(const ModelInfo &model);
+    // delist a model and erase its settings, or mark it as not installed if built-in
-    Q_INVOKABLE void removeInstalled(const ModelInfo &model);
+    void uninstall(const ModelInfo &model);
     ModelInfo defaultModelInfo() const;
 
     void addModel(const QString &id);
@@ -501,7 +501,9 @@ private:
     void updateDataInternal(const QString &id, const QVector<QPair<int, QVariant>> &data, QMutexLocker<QMutex> &lock,
                             bool relock = true);
 
-    void removeInternal(const ModelInfo &model);
+    // call with lock held
+    void removeInternal(const QString &id, QMutexLocker<QMutex> &lock, bool relock = true);
+
     void clearDiscoveredModels();
     bool modelExists(const QString &fileName) const;
     int indexForModel(ModelInfo *model);