From 158f9b9ee7648b4951e3fef1e5e5f4ea4ec27e67 Mon Sep 17 00:00:00 2001 From: Adam Treat Date: Fri, 20 Sep 2024 14:51:27 -0400 Subject: [PATCH 01/15] Revert "modellist: work around filtered item models getting out of sync (#2545)" This is what caused regression seen in issue #2943 This reverts commit 30692a2dfc32569bfee73373815a50bc5a5f97c5. Signed-off-by: Adam Treat --- gpt4all-chat/src/modellist.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/gpt4all-chat/src/modellist.cpp b/gpt4all-chat/src/modellist.cpp index d53c8fbfa316..db74209e220e 100644 --- a/gpt4all-chat/src/modellist.cpp +++ b/gpt4all-chat/src/modellist.cpp @@ -1010,12 +1010,6 @@ void ModelList::updateData(const QString &id, const QVector } } emit dataChanged(createIndex(index, 0), createIndex(index, 0)); - - // FIXME(jared): for some reason these don't update correctly when the source model changes, so we explicitly invalidate them - m_selectableModels->invalidate(); - m_installedModels->invalidate(); - m_downloadableModels->invalidate(); - emit selectableModelListChanged(); } From 6cde7cb671095862ce9e005f504dc47fae529458 Mon Sep 17 00:00:00 2001 From: Jared Van Bortel Date: Fri, 4 Oct 2024 18:05:07 -0400 Subject: [PATCH 02/15] fix double semicolons Signed-off-by: Jared Van Bortel --- gpt4all-chat/src/database.cpp | 2 +- gpt4all-chat/src/modellist.cpp | 6 +++--- gpt4all-chat/src/mysettings.cpp | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/gpt4all-chat/src/database.cpp b/gpt4all-chat/src/database.cpp index 9b1e9ecdb624..f89d3378d33e 100644 --- a/gpt4all-chat/src/database.cpp +++ b/gpt4all-chat/src/database.cpp @@ -1659,7 +1659,7 @@ void Database::scanQueue() if (info.isPdf()) { QPdfDocument doc; if (doc.load(document_path) != QPdfDocument::Error::None) { - qWarning() << "ERROR: Could not load pdf" << document_id << document_path;; + qWarning() << "ERROR: Could not load pdf" << document_id << document_path; return updateFolderToIndex(folder_id, countForFolder); } title = doc.metaData(QPdfDocument::MetaDataField::Title).toString(); diff --git a/gpt4all-chat/src/modellist.cpp b/gpt4all-chat/src/modellist.cpp index db74209e220e..d37894ac57ef 100644 --- a/gpt4all-chat/src/modellist.cpp +++ b/gpt4all-chat/src/modellist.cpp @@ -502,7 +502,7 @@ ModelList::ModelList() connect(MySettings::globalInstance(), &MySettings::contextLengthChanged, this, &ModelList::updateDataForSettings); connect(MySettings::globalInstance(), &MySettings::gpuLayersChanged, this, &ModelList::updateDataForSettings); connect(MySettings::globalInstance(), &MySettings::repeatPenaltyChanged, this, &ModelList::updateDataForSettings); - connect(MySettings::globalInstance(), &MySettings::repeatPenaltyTokensChanged, this, &ModelList::updateDataForSettings);; + connect(MySettings::globalInstance(), &MySettings::repeatPenaltyTokensChanged, this, &ModelList::updateDataForSettings); connect(MySettings::globalInstance(), &MySettings::promptTemplateChanged, this, &ModelList::updateDataForSettings); connect(MySettings::globalInstance(), &MySettings::systemPromptChanged, this, &ModelList::updateDataForSettings); connect(&m_networkManager, &QNetworkAccessManager::sslErrors, this, &ModelList::handleSslErrors); @@ -2094,7 +2094,7 @@ void ModelList::parseDiscoveryJsonFile(const QByteArray &jsonData) emit discoverProgressChanged(); if (!m_discoverNumberOfResults) { m_discoverInProgress = false; - emit discoverInProgressChanged();; + emit discoverInProgressChanged(); } } @@ -2172,7 +2172,7 @@ void ModelList::handleDiscoveryItemFinished() if (discoverProgress() >= 1.0) { emit layoutChanged(); m_discoverInProgress = false; - emit discoverInProgressChanged();; + emit discoverInProgressChanged(); } reply->deleteLater(); diff --git a/gpt4all-chat/src/mysettings.cpp b/gpt4all-chat/src/mysettings.cpp index 97af196fa4ca..38c8ab6821f5 100644 --- a/gpt4all-chat/src/mysettings.cpp +++ b/gpt4all-chat/src/mysettings.cpp @@ -186,7 +186,7 @@ void MySettings::restoreModelDefaults(const ModelInfo &info) setModelTemperature(info, info.m_temperature); setModelTopP(info, info.m_topP); setModelMinP(info, info.m_minP); - setModelTopK(info, info.m_topK);; + setModelTopK(info, info.m_topK); setModelMaxLength(info, info.m_maxLength); setModelPromptBatchSize(info, info.m_promptBatchSize); setModelContextLength(info, info.m_contextLength); From d3840fb817f6474b7964172a4f41e70cfe30597f Mon Sep 17 00:00:00 2001 From: Jared Van Bortel Date: Fri, 4 Oct 2024 18:10:38 -0400 Subject: [PATCH 03/15] modellist: prefer const iterators to avoid detach Signed-off-by: Jared Van Bortel --- gpt4all-chat/src/modellist.cpp | 39 +++++++++++++++++----------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/gpt4all-chat/src/modellist.cpp b/gpt4all-chat/src/modellist.cpp index d37894ac57ef..85f25ca7a39e 100644 --- a/gpt4all-chat/src/modellist.cpp +++ b/gpt4all-chat/src/modellist.cpp @@ -542,7 +542,7 @@ const QList ModelList::selectableModelList() const // FIXME: This needs to be kept in sync with m_selectableModels so should probably be merged QMutexLocker locker(&m_mutex); QList infos; - for (ModelInfo *info : m_models) + for (auto *info : std::as_const(m_models)) if (info->installed && !info->isEmbeddingModel) infos.append(*info); return infos; @@ -560,7 +560,7 @@ ModelInfo ModelList::defaultModelInfo() const const bool hasUserDefaultName = !userDefaultModelName.isEmpty() && userDefaultModelName != "Application default"; ModelInfo *defaultModel = nullptr; - for (ModelInfo *info : m_models) { + for (auto *info : std::as_const(m_models)) { if (!info->installed) continue; defaultModel = info; @@ -589,7 +589,7 @@ bool ModelList::contains(const QString &id) const bool ModelList::containsByFilename(const QString &filename) const { QMutexLocker locker(&m_mutex); - for (ModelInfo *info : m_models) + for (auto *info : std::as_const(m_models)) if (info->filename() == filename) return true; return false; @@ -804,7 +804,7 @@ QVariant ModelList::data(const QString &id, int role) const QVariant ModelList::dataByFilename(const QString &filename, int role) const { QMutexLocker locker(&m_mutex); - for (ModelInfo *info : m_models) + for (auto *info : std::as_const(m_models)) if (info->filename() == filename) return dataInternal(info, role); return QVariant(); @@ -1035,7 +1035,7 @@ void ModelList::updateDataByFilename(const QString &filename, QVector modelsById; { QMutexLocker locker(&m_mutex); - for (ModelInfo *info : m_models) + for (auto *info : std::as_const(m_models)) if (info->filename() == filename) modelsById.append(info->id()); } @@ -1045,7 +1045,7 @@ void ModelList::updateDataByFilename(const QString &filename, QVectorfilename() == filename) return *info; return ModelInfo(); @@ -1069,10 +1069,9 @@ ModelInfo ModelList::modelInfoByFilename(const QString &filename) const bool ModelList::isUniqueName(const QString &name) const { QMutexLocker locker(&m_mutex); - for (const ModelInfo *info : m_models) { - if(info->name() == name) + for (auto *info : std::as_const(m_models)) + if (info->name() == name) return false; - } return true; } @@ -1169,8 +1168,8 @@ QString ModelList::uniqueModelName(const ModelInfo &model) const int maxSuffixNumber = 0; bool baseNameExists = false; - for (const ModelInfo *info : m_models) { - if(info->name() == baseName) + for (auto *info : std::as_const(m_models)) { + if (info->name() == baseName) baseNameExists = true; QRegularExpressionMatch match = re.match(info->name()); @@ -1291,7 +1290,7 @@ void ModelList::processModelDirectory(const QString &path) QVector modelsById; { QMutexLocker locker(&m_mutex); - for (ModelInfo *info : m_models) + for (auto *info : std::as_const(m_models)) if (info->filename() == filename) modelsById.append(info->id()); } @@ -1302,7 +1301,7 @@ void ModelList::processModelDirectory(const QString &path) modelsById.append(filename); } - for (const QString &id : modelsById) { + for (auto &id : std::as_const(modelsById)) { QVector> data { { InstalledRole, true }, { FilenameRole, filename }, @@ -1469,7 +1468,7 @@ void ModelList::parseModelsJsonFile(const QByteArray &jsonData, bool save) QJsonArray jsonArray = document.array(); const QString currentVersion = QCoreApplication::applicationVersion(); - for (const QJsonValue &value : jsonArray) { + for (auto &value : std::as_const(jsonArray)) { QJsonObject obj = value.toObject(); QString modelName = obj["name"].toString(); @@ -1749,7 +1748,7 @@ void ModelList::updateModelsFromSettings() { QSettings settings; QStringList groups = settings.childGroups(); - for (const QString &g: groups) { + for (auto &g : std::as_const(groups)) { if (!g.startsWith("model-")) continue; @@ -1916,11 +1915,11 @@ void ModelList::clearDiscoveredModels() QList infos; { QMutexLocker locker(&m_mutex); - for (ModelInfo *info : m_models) + for (auto *info : std::as_const(m_models)) if (info->isDiscovered() && !info->installed) infos.append(*info); } - for (ModelInfo &info : infos) + for (auto &info : std::as_const(infos)) removeInternal(info); emit layoutChanged(); } @@ -2047,7 +2046,7 @@ void ModelList::parseDiscoveryJsonFile(const QByteArray &jsonData) QJsonArray jsonArray = document.array(); - for (const QJsonValue &value : jsonArray) { + for (auto &value : std::as_const(jsonArray)) { QJsonObject obj = value.toObject(); QJsonDocument jsonDocument(obj); QByteArray jsonData = jsonDocument.toJson(); @@ -2055,7 +2054,7 @@ void ModelList::parseDiscoveryJsonFile(const QByteArray &jsonData) QString repo_id = obj["id"].toString(); QJsonArray siblingsArray = obj["siblings"].toArray(); QList> filteredAndSortedFilenames; - for (const QJsonValue &sibling : siblingsArray) { + for (auto &sibling : std::as_const(siblingsArray)) { QJsonObject s = sibling.toObject(); QString filename = s["rfilename"].toString(); From 45120d4d96e8ab1f9d004e5e7054e056a0d4931c Mon Sep 17 00:00:00 2001 From: Jared Van Bortel Date: Fri, 4 Oct 2024 18:14:11 -0400 Subject: [PATCH 04/15] do not connect layoutChanged to countChanged layoutChanged is only used when the *order* changes. When rows are inserted or removed, one of the other three signals is fired. Signed-off-by: Jared Van Bortel --- gpt4all-chat/src/localdocsmodel.cpp | 2 -- gpt4all-chat/src/modellist.cpp | 2 -- 2 files changed, 4 deletions(-) diff --git a/gpt4all-chat/src/localdocsmodel.cpp b/gpt4all-chat/src/localdocsmodel.cpp index fba4c4ff32dc..a10773625e0c 100644 --- a/gpt4all-chat/src/localdocsmodel.cpp +++ b/gpt4all-chat/src/localdocsmodel.cpp @@ -20,7 +20,6 @@ LocalDocsCollectionsModel::LocalDocsCollectionsModel(QObject *parent) connect(this, &LocalDocsCollectionsModel::rowsInserted, this, &LocalDocsCollectionsModel::countChanged); connect(this, &LocalDocsCollectionsModel::rowsRemoved, this, &LocalDocsCollectionsModel::countChanged); connect(this, &LocalDocsCollectionsModel::modelReset, this, &LocalDocsCollectionsModel::countChanged); - connect(this, &LocalDocsCollectionsModel::layoutChanged, this, &LocalDocsCollectionsModel::countChanged); } bool LocalDocsCollectionsModel::filterAcceptsRow(int sourceRow, @@ -67,7 +66,6 @@ LocalDocsModel::LocalDocsModel(QObject *parent) connect(this, &LocalDocsModel::rowsInserted, this, &LocalDocsModel::countChanged); connect(this, &LocalDocsModel::rowsRemoved, this, &LocalDocsModel::countChanged); connect(this, &LocalDocsModel::modelReset, this, &LocalDocsModel::countChanged); - connect(this, &LocalDocsModel::layoutChanged, this, &LocalDocsModel::countChanged); } int LocalDocsModel::rowCount(const QModelIndex &parent) const diff --git a/gpt4all-chat/src/modellist.cpp b/gpt4all-chat/src/modellist.cpp index 85f25ca7a39e..226c72024a97 100644 --- a/gpt4all-chat/src/modellist.cpp +++ b/gpt4all-chat/src/modellist.cpp @@ -398,7 +398,6 @@ InstalledModels::InstalledModels(QObject *parent, bool selectable) connect(this, &InstalledModels::rowsInserted, this, &InstalledModels::countChanged); connect(this, &InstalledModels::rowsRemoved, this, &InstalledModels::countChanged); connect(this, &InstalledModels::modelReset, this, &InstalledModels::countChanged); - connect(this, &InstalledModels::layoutChanged, this, &InstalledModels::countChanged); } bool InstalledModels::filterAcceptsRow(int sourceRow, @@ -423,7 +422,6 @@ DownloadableModels::DownloadableModels(QObject *parent) connect(this, &DownloadableModels::rowsInserted, this, &DownloadableModels::countChanged); connect(this, &DownloadableModels::rowsRemoved, this, &DownloadableModels::countChanged); connect(this, &DownloadableModels::modelReset, this, &DownloadableModels::countChanged); - connect(this, &DownloadableModels::layoutChanged, this, &DownloadableModels::countChanged); } bool DownloadableModels::filterAcceptsRow(int sourceRow, From 3dea7e2bfa54f5e76edb4c7dd8f519e7a73c2397 Mon Sep 17 00:00:00 2001 From: Jared Van Bortel Date: Fri, 4 Oct 2024 18:18:13 -0400 Subject: [PATCH 05/15] modellist: fix missing #include Signed-off-by: Jared Van Bortel --- gpt4all-chat/src/modellist.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/gpt4all-chat/src/modellist.cpp b/gpt4all-chat/src/modellist.cpp index 226c72024a97..4d2bd17ceaf7 100644 --- a/gpt4all-chat/src/modellist.cpp +++ b/gpt4all-chat/src/modellist.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include From d0158d2013a18381d6f1a9384c8cf8ac2372d4de Mon Sep 17 00:00:00 2001 From: Jared Van Bortel Date: Fri, 4 Oct 2024 18:29:15 -0400 Subject: [PATCH 06/15] modellist: assert that update functions are called from main thread The locking strategy used by ModelList assumes that only one thread will be trying to update the model list at a time, as writes cannot be implemented in a fully threadsafe manner. Signed-off-by: Jared Van Bortel --- gpt4all-chat/src/modellist.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/gpt4all-chat/src/modellist.cpp b/gpt4all-chat/src/modellist.cpp index 4d2bd17ceaf7..22d32b580d1c 100644 --- a/gpt4all-chat/src/modellist.cpp +++ b/gpt4all-chat/src/modellist.cpp @@ -632,6 +632,7 @@ bool ModelList::lessThan(const ModelInfo* a, const ModelInfo* b, DiscoverSort s, void ModelList::addModel(const QString &id) { + Q_ASSERT(QThread::currentThread() == thread()); const bool hasModel = contains(id); Q_ASSERT(!hasModel); if (hasModel) { @@ -820,6 +821,7 @@ QVariant ModelList::data(const QModelIndex &index, int role) const void ModelList::updateData(const QString &id, const QVector> &data) { + Q_ASSERT(QThread::currentThread() == thread()); int index; { QMutexLocker locker(&m_mutex); @@ -1028,6 +1030,7 @@ void ModelList::resortModel() void ModelList::updateDataByFilename(const QString &filename, QVector> data) { + Q_ASSERT(QThread::currentThread() == thread()); if (data.isEmpty()) return; // no-op @@ -1076,6 +1079,7 @@ bool ModelList::isUniqueName(const QString &name) const QString ModelList::clone(const ModelInfo &model) { + Q_ASSERT(QThread::currentThread() == thread()); const QString id = Network::globalInstance()->generateUniqueId(); addModel(id); @@ -1109,6 +1113,7 @@ QString ModelList::clone(const ModelInfo &model) void ModelList::removeClone(const ModelInfo &model) { + Q_ASSERT(QThread::currentThread() == thread()); Q_ASSERT(model.isClone()); if (!model.isClone()) return; @@ -1119,6 +1124,7 @@ void ModelList::removeClone(const ModelInfo &model) void ModelList::removeInstalled(const ModelInfo &model) { + Q_ASSERT(QThread::currentThread() == thread()); Q_ASSERT(model.installed); Q_ASSERT(!model.isClone()); Q_ASSERT(model.isDiscovered() || model.isCompatibleApi || model.description() == "" /*indicates sideloaded*/); @@ -1128,6 +1134,7 @@ void ModelList::removeInstalled(const ModelInfo &model) void ModelList::removeInternal(const ModelInfo &model) { + Q_ASSERT(QThread::currentThread() == thread()); const bool hasModel = contains(model.id()); Q_ASSERT(hasModel); if (!hasModel) { @@ -1325,6 +1332,7 @@ void ModelList::processModelDirectory(const QString &path) void ModelList::updateModelsFromDirectory() { + Q_ASSERT(QThread::currentThread() == thread()); const QString exePath = QCoreApplication::applicationDirPath() + QDir::separator(); const QString localPath = MySettings::globalInstance()->modelPath(); @@ -1727,6 +1735,7 @@ void ModelList::parseModelsJsonFile(const QByteArray &jsonData, bool save) void ModelList::updateDiscoveredInstalled(const ModelInfo &info) { + Q_ASSERT(QThread::currentThread() == thread()); QVector> data { { ModelList::InstalledRole, true }, { ModelList::IsDiscoveredRole, true }, @@ -1937,6 +1946,7 @@ bool ModelList::discoverInProgress() const void ModelList::discoverSearch(const QString &search) { + Q_ASSERT(QThread::currentThread() == thread()); Q_ASSERT(!m_discoverInProgress); clearDiscoveredModels(); From 9ec0f92f67df67af9f84d0f6a76b8f481ab29a61 Mon Sep 17 00:00:00 2001 From: Jared Van Bortel Date: Fri, 4 Oct 2024 18:36:17 -0400 Subject: [PATCH 07/15] mysettings: eraseModel only needs an id Signed-off-by: Jared Van Bortel --- gpt4all-chat/src/download.cpp | 2 +- gpt4all-chat/src/modellist.cpp | 2 +- gpt4all-chat/src/mysettings.cpp | 4 ++-- gpt4all-chat/src/mysettings.h | 3 ++- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/gpt4all-chat/src/download.cpp b/gpt4all-chat/src/download.cpp index e773a41fe385..62ff73fb3da2 100644 --- a/gpt4all-chat/src/download.cpp +++ b/gpt4all-chat/src/download.cpp @@ -340,7 +340,7 @@ void Download::removeModel(const QString &modelFile) QFile file(filePath); if (file.exists()) { const ModelInfo info = ModelList::globalInstance()->modelInfoByFilename(modelFile); - MySettings::globalInstance()->eraseModel(info); + MySettings::globalInstance()->eraseModel(info.id()); shouldRemoveInstalled = info.installed && !info.isClone() && (info.isDiscovered() || info.isCompatibleApi || info.description() == "" /*indicates sideloaded*/); if (shouldRemoveInstalled) ModelList::globalInstance()->removeInstalled(info); diff --git a/gpt4all-chat/src/modellist.cpp b/gpt4all-chat/src/modellist.cpp index 22d32b580d1c..4e52c4b08caf 100644 --- a/gpt4all-chat/src/modellist.cpp +++ b/gpt4all-chat/src/modellist.cpp @@ -1157,7 +1157,7 @@ void ModelList::removeInternal(const ModelInfo &model) } endRemoveRows(); emit selectableModelListChanged(); - MySettings::globalInstance()->eraseModel(model); + MySettings::globalInstance()->eraseModel(model.id()); } QString ModelList::uniqueModelName(const ModelInfo &model) const diff --git a/gpt4all-chat/src/mysettings.cpp b/gpt4all-chat/src/mysettings.cpp index 38c8ab6821f5..553d2930d07b 100644 --- a/gpt4all-chat/src/mysettings.cpp +++ b/gpt4all-chat/src/mysettings.cpp @@ -226,9 +226,9 @@ void MySettings::restoreLocalDocsDefaults() setLocalDocsEmbedDevice(basicDefaults.value("localdocs/embedDevice").toString()); } -void MySettings::eraseModel(const ModelInfo &info) +void MySettings::eraseModel(QStringView id) { - m_settings.remove(u"model-%1"_s.arg(info.id())); + m_settings.remove(u"model-%1"_s.arg(id)); } QString MySettings::modelName(const ModelInfo &info) const diff --git a/gpt4all-chat/src/mysettings.h b/gpt4all-chat/src/mysettings.h index 85335f0b0696..474c7f3d0ed2 100644 --- a/gpt4all-chat/src/mysettings.h +++ b/gpt4all-chat/src/mysettings.h @@ -8,6 +8,7 @@ #include #include #include +#include #include #include @@ -83,7 +84,7 @@ class MySettings : public QObject Q_INVOKABLE void restoreLocalDocsDefaults(); // Model/Character settings - void eraseModel(const ModelInfo &info); + void eraseModel(QStringView id); QString modelName(const ModelInfo &info) const; Q_INVOKABLE void setModelName(const ModelInfo &info, const QString &name, bool force = false); QString modelFilename(const ModelInfo &info) const; From 62186a007e09cdb0a0822e3f9413a39f632c51f5 Mon Sep 17 00:00:00 2001 From: Jared Van Bortel Date: Fri, 4 Oct 2024 18:42:47 -0400 Subject: [PATCH 08/15] modellist: reduce updateData indent Signed-off-by: Jared Van Bortel --- gpt4all-chat/src/modellist.cpp | 355 +++++++++++++++++---------------- gpt4all-chat/src/modellist.h | 6 + 2 files changed, 188 insertions(+), 173 deletions(-) diff --git a/gpt4all-chat/src/modellist.cpp b/gpt4all-chat/src/modellist.cpp index 4e52c4b08caf..795773612db4 100644 --- a/gpt4all-chat/src/modellist.cpp +++ b/gpt4all-chat/src/modellist.cpp @@ -820,198 +820,207 @@ QVariant ModelList::data(const QModelIndex &index, int role) const } void ModelList::updateData(const QString &id, const QVector> &data) +{ + QMutexLocker lock(&m_mutex); + updateDataInternal(id, data, lock, /*relock*/ false); +} + +void ModelList::updateDataInternal(const QString &id, const QVector> &data, + QMutexLocker &lock, bool relock) { Q_ASSERT(QThread::currentThread() == thread()); - int index; - { - QMutexLocker locker(&m_mutex); - if (!m_modelMap.contains(id)) { - qWarning() << "ERROR: cannot update as model map does not contain" << id; - return; - } + Q_ASSERT(lock.isLocked()); - ModelInfo *info = m_modelMap.value(id); - index = m_models.indexOf(info); - if (index == -1) { - qWarning() << "ERROR: cannot update as model list does not contain" << id; - return; - } + if (!m_modelMap.contains(id)) { + qWarning() << "ERROR: cannot update as model map does not contain" << id; + return; + } + + ModelInfo *info = m_modelMap.value(id); + int index = m_models.indexOf(info); + if (index == -1) { + qWarning() << "ERROR: cannot update as model list does not contain" << id; + return; + } + + // We only sort when one of the fields used by the sorting algorithm actually changes that + // is implicated or used by the sorting algorithm + bool shouldSort = false; - // We only sort when one of the fields used by the sorting algorithm actually changes that - // is implicated or used by the sorting algorithm - bool shouldSort = false; - - for (const auto &d : data) { - const int role = d.first; - const QVariant value = d.second; - switch (role) { - case IdRole: - { - if (info->id() != value.toString()) { - info->setId(value.toString()); - shouldSort = true; - } - break; + for (const auto &d : data) { + const int role = d.first; + const QVariant value = d.second; + switch (role) { + case IdRole: + { + if (info->id() != value.toString()) { + info->setId(value.toString()); + shouldSort = true; } - case NameRole: - info->setName(value.toString()); break; - case FilenameRole: - info->setFilename(value.toString()); break; - case DirpathRole: - info->dirpath = value.toString(); break; - case FilesizeRole: - info->filesize = value.toString(); break; - case HashRole: - info->hash = value.toByteArray(); break; - case HashAlgorithmRole: - info->hashAlgorithm = static_cast(value.toInt()); break; - case CalcHashRole: - info->calcHash = value.toBool(); break; - case InstalledRole: - info->installed = value.toBool(); break; - case DefaultRole: - info->isDefault = value.toBool(); break; - case OnlineRole: - info->isOnline = value.toBool(); break; - case CompatibleApiRole: - info->isCompatibleApi = value.toBool(); break; - case DescriptionRole: - info->setDescription(value.toString()); break; - case RequiresVersionRole: - info->requiresVersion = value.toString(); break; - case VersionRemovedRole: - info->versionRemoved = value.toString(); break; - case UrlRole: - info->setUrl(value.toString()); break; - case BytesReceivedRole: - info->bytesReceived = value.toLongLong(); break; - case BytesTotalRole: - info->bytesTotal = value.toLongLong(); break; - case TimestampRole: - info->timestamp = value.toLongLong(); break; - case SpeedRole: - info->speed = value.toString(); break; - case DownloadingRole: - info->isDownloading = value.toBool(); break; - case IncompleteRole: - info->isIncomplete = value.toBool(); break; - case DownloadErrorRole: - info->downloadError = value.toString(); break; - case OrderRole: - { - if (info->order != value.toString()) { - info->order = value.toString(); - shouldSort = true; - } - break; + break; + } + case NameRole: + info->setName(value.toString()); break; + case FilenameRole: + info->setFilename(value.toString()); break; + case DirpathRole: + info->dirpath = value.toString(); break; + case FilesizeRole: + info->filesize = value.toString(); break; + case HashRole: + info->hash = value.toByteArray(); break; + case HashAlgorithmRole: + info->hashAlgorithm = static_cast(value.toInt()); break; + case CalcHashRole: + info->calcHash = value.toBool(); break; + case InstalledRole: + info->installed = value.toBool(); break; + case DefaultRole: + info->isDefault = value.toBool(); break; + case OnlineRole: + info->isOnline = value.toBool(); break; + case CompatibleApiRole: + info->isCompatibleApi = value.toBool(); break; + case DescriptionRole: + info->setDescription(value.toString()); break; + case RequiresVersionRole: + info->requiresVersion = value.toString(); break; + case VersionRemovedRole: + info->versionRemoved = value.toString(); break; + case UrlRole: + info->setUrl(value.toString()); break; + case BytesReceivedRole: + info->bytesReceived = value.toLongLong(); break; + case BytesTotalRole: + info->bytesTotal = value.toLongLong(); break; + case TimestampRole: + info->timestamp = value.toLongLong(); break; + case SpeedRole: + info->speed = value.toString(); break; + case DownloadingRole: + info->isDownloading = value.toBool(); break; + case IncompleteRole: + info->isIncomplete = value.toBool(); break; + case DownloadErrorRole: + info->downloadError = value.toString(); break; + case OrderRole: + { + if (info->order != value.toString()) { + info->order = value.toString(); + shouldSort = true; } - case RamrequiredRole: - info->ramrequired = value.toInt(); break; - case ParametersRole: - info->parameters = value.toString(); break; - case QuantRole: - info->setQuant(value.toString()); break; - case TypeRole: - info->setType(value.toString()); break; - case IsCloneRole: - { - if (info->isClone() != value.toBool()) { - info->setIsClone(value.toBool()); - shouldSort = true; - } - break; + break; + } + case RamrequiredRole: + info->ramrequired = value.toInt(); break; + case ParametersRole: + info->parameters = value.toString(); break; + case QuantRole: + info->setQuant(value.toString()); break; + case TypeRole: + info->setType(value.toString()); break; + case IsCloneRole: + { + if (info->isClone() != value.toBool()) { + info->setIsClone(value.toBool()); + shouldSort = true; } - case IsDiscoveredRole: - { - if (info->isDiscovered() != value.toBool()) { - info->setIsDiscovered(value.toBool()); - shouldSort = true; - } - break; + break; + } + case IsDiscoveredRole: + { + if (info->isDiscovered() != value.toBool()) { + info->setIsDiscovered(value.toBool()); + shouldSort = true; } - case IsEmbeddingModelRole: - info->isEmbeddingModel = value.toBool(); break; - case TemperatureRole: - info->setTemperature(value.toDouble()); break; - case TopPRole: - info->setTopP(value.toDouble()); break; - case MinPRole: - info->setMinP(value.toDouble()); break; - case TopKRole: - info->setTopK(value.toInt()); break; - case MaxLengthRole: - info->setMaxLength(value.toInt()); break; - case PromptBatchSizeRole: - info->setPromptBatchSize(value.toInt()); break; - case ContextLengthRole: - info->setContextLength(value.toInt()); break; - case GpuLayersRole: - info->setGpuLayers(value.toInt()); break; - case RepeatPenaltyRole: - info->setRepeatPenalty(value.toDouble()); break; - case RepeatPenaltyTokensRole: - info->setRepeatPenaltyTokens(value.toInt()); break; - case PromptTemplateRole: - info->setPromptTemplate(value.toString()); break; - case SystemPromptRole: - info->setSystemPrompt(value.toString()); break; - case ChatNamePromptRole: - info->setChatNamePrompt(value.toString()); break; - case SuggestedFollowUpPromptRole: - info->setSuggestedFollowUpPrompt(value.toString()); break; - case LikesRole: - { - if (info->likes() != value.toInt()) { - info->setLikes(value.toInt()); - shouldSort = true; - } - break; + break; + } + case IsEmbeddingModelRole: + info->isEmbeddingModel = value.toBool(); break; + case TemperatureRole: + info->setTemperature(value.toDouble()); break; + case TopPRole: + info->setTopP(value.toDouble()); break; + case MinPRole: + info->setMinP(value.toDouble()); break; + case TopKRole: + info->setTopK(value.toInt()); break; + case MaxLengthRole: + info->setMaxLength(value.toInt()); break; + case PromptBatchSizeRole: + info->setPromptBatchSize(value.toInt()); break; + case ContextLengthRole: + info->setContextLength(value.toInt()); break; + case GpuLayersRole: + info->setGpuLayers(value.toInt()); break; + case RepeatPenaltyRole: + info->setRepeatPenalty(value.toDouble()); break; + case RepeatPenaltyTokensRole: + info->setRepeatPenaltyTokens(value.toInt()); break; + case PromptTemplateRole: + info->setPromptTemplate(value.toString()); break; + case SystemPromptRole: + info->setSystemPrompt(value.toString()); break; + case ChatNamePromptRole: + info->setChatNamePrompt(value.toString()); break; + case SuggestedFollowUpPromptRole: + info->setSuggestedFollowUpPrompt(value.toString()); break; + case LikesRole: + { + if (info->likes() != value.toInt()) { + info->setLikes(value.toInt()); + shouldSort = true; } - case DownloadsRole: - { - if (info->downloads() != value.toInt()) { - info->setDownloads(value.toInt()); - shouldSort = true; - } - break; + break; + } + case DownloadsRole: + { + if (info->downloads() != value.toInt()) { + info->setDownloads(value.toInt()); + shouldSort = true; } - case RecencyRole: - { - if (info->recency() != value.toDateTime()) { - info->setRecency(value.toDateTime()); - shouldSort = true; - } - break; + break; + } + case RecencyRole: + { + if (info->recency() != value.toDateTime()) { + info->setRecency(value.toDateTime()); + shouldSort = true; } + break; } } + } - // Extra guarantee that these always remains in sync with filesystem - QString modelPath = info->dirpath + info->filename(); - const QFileInfo fileInfo(modelPath); - info->installed = fileInfo.exists(); - const QFileInfo incompleteInfo(incompleteDownloadPath(info->filename())); - info->isIncomplete = incompleteInfo.exists(); + // Extra guarantee that these always remains in sync with filesystem + QString modelPath = info->dirpath + info->filename(); + const QFileInfo fileInfo(modelPath); + info->installed = fileInfo.exists(); + const QFileInfo incompleteInfo(incompleteDownloadPath(info->filename())); + info->isIncomplete = incompleteInfo.exists(); - // check installed, discovered/sideloaded models only (including clones) - if (!info->checkedEmbeddingModel && !info->isEmbeddingModel && info->installed - && (info->isDiscovered() || info->description().isEmpty())) - { - // read GGUF and decide based on model architecture - info->isEmbeddingModel = LLModel::Implementation::isEmbeddingModel(modelPath.toStdString()); - info->checkedEmbeddingModel = true; - } + // check installed, discovered/sideloaded models only (including clones) + if (!info->checkedEmbeddingModel && !info->isEmbeddingModel && info->installed + && (info->isDiscovered() || info->description().isEmpty())) + { + // read GGUF and decide based on model architecture + info->isEmbeddingModel = LLModel::Implementation::isEmbeddingModel(modelPath.toStdString()); + info->checkedEmbeddingModel = true; + } - if (shouldSort) { - auto s = m_discoverSort; - auto d = m_discoverSortDirection; - std::stable_sort(m_models.begin(), m_models.end(), [s, d](const ModelInfo* lhs, const ModelInfo* rhs) { - return ModelList::lessThan(lhs, rhs, s, d); - }); - } + if (shouldSort) { + auto s = m_discoverSort; + auto d = m_discoverSortDirection; + std::stable_sort(m_models.begin(), m_models.end(), [s, d](const ModelInfo* lhs, const ModelInfo* rhs) { + return ModelList::lessThan(lhs, rhs, s, d); + }); } + + lock.unlock(); emit dataChanged(createIndex(index, 0), createIndex(index, 0)); emit selectableModelListChanged(); + if (relock) + lock.relock(); } void ModelList::resortModel() diff --git a/gpt4all-chat/src/modellist.h b/gpt4all-chat/src/modellist.h index 6123dde81b6c..3431f8c56244 100644 --- a/gpt4all-chat/src/modellist.h +++ b/gpt4all-chat/src/modellist.h @@ -23,6 +23,8 @@ using namespace Qt::Literals::StringLiterals; +template class QMutexLocker; + struct ModelInfo { Q_GADGET @@ -495,6 +497,10 @@ private Q_SLOTS: void handleSslErrors(QNetworkReply *reply, const QList &errors); private: + // call with lock held + void updateDataInternal(const QString &id, const QVector> &data, QMutexLocker &lock, + bool relock = true); + void removeInternal(const ModelInfo &model); void clearDiscoveredModels(); bool modelExists(const QString &fileName) const; From 348c0515d8d6bead8ab8e574100a7151a8244b66 Mon Sep 17 00:00:00 2001 From: Jared Van Bortel Date: Fri, 4 Oct 2024 18:48:42 -0400 Subject: [PATCH 09/15] modellist: emit layoutChanged when sorting, but not otherwise Also, hint to layoutChanged about the specific way in which the model was sorted. Signed-off-by: Jared Van Bortel --- gpt4all-chat/src/modellist.cpp | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/gpt4all-chat/src/modellist.cpp b/gpt4all-chat/src/modellist.cpp index 795773612db4..51af296cadae 100644 --- a/gpt4all-chat/src/modellist.cpp +++ b/gpt4all-chat/src/modellist.cpp @@ -1008,24 +1008,22 @@ void ModelList::updateDataInternal(const QString &id, const QVectorcheckedEmbeddingModel = true; } - if (shouldSort) { - auto s = m_discoverSort; - auto d = m_discoverSortDirection; - std::stable_sort(m_models.begin(), m_models.end(), [s, d](const ModelInfo* lhs, const ModelInfo* rhs) { - return ModelList::lessThan(lhs, rhs, s, d); - }); - } - lock.unlock(); + + if (shouldSort) + resortModel(); + emit dataChanged(createIndex(index, 0), createIndex(index, 0)); emit selectableModelListChanged(); + if (relock) lock.relock(); } void ModelList::resortModel() { - emit layoutAboutToBeChanged(); + const QList parents { QModelIndex() }; + emit layoutAboutToBeChanged(parents, QAbstractItemModel::VerticalSortHint); { QMutexLocker locker(&m_mutex); auto s = m_discoverSort; @@ -1034,7 +1032,7 @@ void ModelList::resortModel() return ModelList::lessThan(lhs, rhs, s, d); }); } - emit layoutChanged(); + emit layoutChanged(parents, QAbstractItemModel::VerticalSortHint); } void ModelList::updateDataByFilename(const QString &filename, QVector> data) @@ -1938,7 +1936,6 @@ void ModelList::clearDiscoveredModels() } for (auto &info : std::as_const(infos)) removeInternal(info); - emit layoutChanged(); } float ModelList::discoverProgress() const @@ -2187,7 +2184,6 @@ void ModelList::handleDiscoveryItemFinished() emit discoverProgressChanged(); if (discoverProgress() >= 1.0) { - emit layoutChanged(); m_discoverInProgress = false; emit discoverInProgressChanged(); } From 3cfcb2a4d68fd19f9db6ad5a6190c5556be2a9de Mon Sep 17 00:00:00 2001 From: Jared Van Bortel Date: Fri, 4 Oct 2024 18:49:43 -0400 Subject: [PATCH 10/15] modellist: emit dataChanged on the correct index dataChanged will not work correctly if we emit it on an old index after sorting. Signed-off-by: Jared Van Bortel --- gpt4all-chat/src/modellist.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gpt4all-chat/src/modellist.cpp b/gpt4all-chat/src/modellist.cpp index 51af296cadae..17538349230d 100644 --- a/gpt4all-chat/src/modellist.cpp +++ b/gpt4all-chat/src/modellist.cpp @@ -1010,10 +1010,10 @@ void ModelList::updateDataInternal(const QString &id, const QVector Date: Fri, 4 Oct 2024 11:15:26 -0400 Subject: [PATCH 11/15] 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 --- gpt4all-chat/qml/ModelSettings.qml | 2 +- gpt4all-chat/src/download.cpp | 39 ++++----- gpt4all-chat/src/modellist.cpp | 130 ++++++++++++++++++----------- gpt4all-chat/src/modellist.h | 8 +- 4 files changed, 101 insertions(+), 78 deletions(-) diff --git a/gpt4all-chat/qml/ModelSettings.qml b/gpt4all-chat/qml/ModelSettings.qml index 2435e08f8b5d..4cbc5b569a3a 100644 --- a/gpt4all-chat/qml/ModelSettings.qml +++ b/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; } } diff --git a/gpt4all-chat/src/download.cpp b/gpt4all-chat/src/download.cpp index 62ff73fb3da2..7d183768f131 100644 --- a/gpt4all-chat/src/download.cpp +++ b/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; - QFile incompleteFile(ModelList::globalInstance()->incompleteDownloadPath(modelFile)); - if (incompleteFile.exists()) { + auto *modelList = ModelList::globalInstance(); + auto *mySettings = MySettings::globalInstance(); + + 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); - MySettings::globalInstance()->eraseModel(info.id()); - shouldRemoveInstalled = info.installed && !info.isClone() && (info.isDiscovered() || info.isCompatibleApi || info.description() == "" /*indicates sideloaded*/); - 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())); + modelList->uninstall(info); + if (!info.isClone()) + file.remove(); } - if (!shouldRemoveInstalled) { - QVector> data { - { ModelList::InstalledRole, false }, - { ModelList::BytesReceivedRole, 0 }, - { ModelList::BytesTotalRole, 0 }, - { ModelList::TimestampRole, 0 }, - { ModelList::SpeedRole, QString() }, - { ModelList::DownloadErrorRole, QString() }, - }; - ModelList::globalInstance()->updateDataByFilename(modelFile, data); - } + emit toastMessage(tr("Model \"%1\" is removed.").arg(info.name())); } void Download::handleSslErrors(QNetworkReply *reply, const QList &errors) diff --git a/gpt4all-chat/src/modellist.cpp b/gpt4all-chat/src/modellist.cpp index 17538349230d..4867ef633da2 100644 --- a/gpt4all-chat/src/modellist.cpp +++ b/gpt4all-chat/src/modellist.cpp @@ -1041,21 +1041,21 @@ void ModelList::updateDataByFilename(const QString &filename, QVector 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()) { - qWarning() << "ERROR: cannot update model as list does not contain file" << filename; - return; - } + if (modelsById.isEmpty()) { + qWarning() << "ERROR: cannot update model as list does not contain file" << filename; + return; + } - for (auto &id : std::as_const(modelsById)) - updateData(id, data); + for (auto &id : std::as_const(modelsById)) + 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); -void ModelList::removeInstalled(const ModelInfo &model) -{ - Q_ASSERT(QThread::currentThread() == thread()); - Q_ASSERT(model.installed); - Q_ASSERT(!model.isClone()); - Q_ASSERT(model.isDiscovered() || model.isCompatibleApi || model.description() == "" /*indicates sideloaded*/); - removeInternal(model); - emit layoutChanged(); + 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> 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::removeInternal(const ModelInfo &model) +void ModelList::removeInternal(const QString &id, QMutexLocker &lock, bool relock) { Q_ASSERT(QThread::currentThread() == thread()); - const bool hasModel = contains(model.id()); + Q_ASSERT(lock.isLocked()); + + bool hasModel = m_modelMap.contains(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; - { - QMutexLocker locker(&m_mutex); - ModelInfo *info = m_modelMap.value(model.id()); - indexOfModel = m_models.indexOf(info); - } - beginRemoveRows(QModelIndex(), indexOfModel, indexOfModel); - { - QMutexLocker locker(&m_mutex); - ModelInfo *info = m_models.takeAt(indexOfModel); - m_modelMap.remove(info->id()); - delete info; - } + auto mapIt = std::as_const(m_modelMap).find(id); + qsizetype listIdx = m_models.indexOf(*mapIt); + + lock.unlock(); + beginRemoveRows({}, listIdx, listIdx); + lock.relock(); + + // remove entry + auto *info = *mapIt; + Q_ASSERT(std::as_const(m_modelMap).find(id) == mapIt); + Q_ASSERT(m_models.indexOf(info) == listIdx); + 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 infos; - { - QMutexLocker locker(&m_mutex); - for (auto *info : std::as_const(m_models)) - if (info->isDiscovered() && !info->installed) - infos.append(*info); - } - for (auto &info : std::as_const(infos)) - removeInternal(info); + QMutexLocker locker(&m_mutex); + QStringList ids; + for (auto *info : std::as_const(m_models)) + if (info->isDiscovered() && !info->installed) + ids << info->id(); + + for (auto &id : std::as_const(ids)) + removeInternal(id, locker, /*relock*/ &id != &ids.constLast()); } float ModelList::discoverProgress() const diff --git a/gpt4all-chat/src/modellist.h b/gpt4all-chat/src/modellist.h index 3431f8c56244..9292d31bac97 100644 --- a/gpt4all-chat/src/modellist.h +++ b/gpt4all-chat/src/modellist.h @@ -421,8 +421,8 @@ class ModelList : public QAbstractListModel 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); - Q_INVOKABLE void removeInstalled(const ModelInfo &model); + // delist a model and erase its settings, or mark it as not installed if built-in + void uninstall(const ModelInfo &model); ModelInfo defaultModelInfo() const; void addModel(const QString &id); @@ -501,7 +501,9 @@ private Q_SLOTS: void updateDataInternal(const QString &id, const QVector> &data, QMutexLocker &lock, bool relock = true); - void removeInternal(const ModelInfo &model); + // call with lock held + void removeInternal(const QString &id, QMutexLocker &lock, bool relock = true); + void clearDiscoveredModels(); bool modelExists(const QString &fileName) const; int indexForModel(ModelInfo *model); From f8143361d31b1cfbfcbc735a265ed2748d039420 Mon Sep 17 00:00:00 2001 From: Jared Van Bortel Date: Mon, 7 Oct 2024 13:40:25 -0400 Subject: [PATCH 12/15] modellist: provide a path property for convenience This should always be used instead of joining the current model path setting with the filename, as models may be in subdirectories. But fixing this correctly is not in scope for this PR. Signed-off-by: Jared Van Bortel --- gpt4all-chat/src/modellist.cpp | 6 +++--- gpt4all-chat/src/modellist.h | 5 +++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/gpt4all-chat/src/modellist.cpp b/gpt4all-chat/src/modellist.cpp index 4867ef633da2..9a39b4ba9400 100644 --- a/gpt4all-chat/src/modellist.cpp +++ b/gpt4all-chat/src/modellist.cpp @@ -258,7 +258,7 @@ int ModelInfo::maxContextLength() const { if (!installed || isOnline) return -1; if (m_maxContextLength != -1) return m_maxContextLength; - auto path = (dirpath + filename()).toStdString(); + auto path = this->path().toStdString(); int n_ctx = LLModel::Implementation::maxContextLength(path); if (n_ctx < 0) { n_ctx = 4096; // fallback value @@ -282,7 +282,7 @@ int ModelInfo::maxGpuLayers() const { if (!installed || isOnline) return -1; if (m_maxGpuLayers != -1) return m_maxGpuLayers; - auto path = (dirpath + filename()).toStdString(); + auto path = this->path().toStdString(); int layers = LLModel::Implementation::layerCount(path); if (layers < 0) { layers = 100; // fallback value @@ -993,7 +993,7 @@ void ModelList::updateDataInternal(const QString &id, const QVectordirpath + info->filename(); + QString modelPath = info->path(); const QFileInfo fileInfo(modelPath); info->installed = fileInfo.exists(); const QFileInfo incompleteInfo(incompleteDownloadPath(info->filename())); diff --git a/gpt4all-chat/src/modellist.h b/gpt4all-chat/src/modellist.h index 9292d31bac97..9f14f413b2f3 100644 --- a/gpt4all-chat/src/modellist.h +++ b/gpt4all-chat/src/modellist.h @@ -31,6 +31,7 @@ struct ModelInfo { Q_PROPERTY(QString id READ id WRITE setId) Q_PROPERTY(QString name READ name WRITE setName) Q_PROPERTY(QString filename READ filename WRITE setFilename) + Q_PROPERTY(QString path READ path) Q_PROPERTY(QString dirpath MEMBER dirpath) Q_PROPERTY(QString filesize MEMBER filesize) Q_PROPERTY(QByteArray hash MEMBER hash) @@ -94,6 +95,9 @@ struct ModelInfo { QString filename() const; void setFilename(const QString &name); + // FIXME(jared): This is the one true path of the model. Never use anything else. + QString path() const { return dirpath + filename(); } + QString description() const; void setDescription(const QString &d); @@ -121,6 +125,7 @@ struct ModelInfo { QDateTime recency() const; void setRecency(const QDateTime &r); + // FIXME(jared): a class with getters should not also have public mutable fields QString dirpath; QString filesize; QByteArray hash; From 8a64b039dcfcde66ea36ffa7f6f085634d8067f2 Mon Sep 17 00:00:00 2001 From: Jared Van Bortel Date: Mon, 7 Oct 2024 13:42:19 -0400 Subject: [PATCH 13/15] fix removal of models in subdirectories removeModel is starting to make sense now. Signed-off-by: Jared Van Bortel --- gpt4all-chat/qml/AddModelView.qml | 4 +--- gpt4all-chat/qml/ModelsView.qml | 4 +--- gpt4all-chat/src/download.cpp | 29 +++++++++++++++-------------- gpt4all-chat/src/download.h | 2 +- 4 files changed, 18 insertions(+), 21 deletions(-) diff --git a/gpt4all-chat/qml/AddModelView.qml b/gpt4all-chat/qml/AddModelView.qml index d366b8f9626c..41bdfd4cfe59 100644 --- a/gpt4all-chat/qml/AddModelView.qml +++ b/gpt4all-chat/qml/AddModelView.qml @@ -441,9 +441,7 @@ Rectangle { Layout.alignment: Qt.AlignTop | Qt.AlignHCenter visible: !isDownloading && (installed || isIncomplete) Accessible.description: qsTr("Remove model from filesystem") - onClicked: { - Download.removeModel(filename); - } + onClicked: Download.removeModel(id) } MySettingsButton { diff --git a/gpt4all-chat/qml/ModelsView.qml b/gpt4all-chat/qml/ModelsView.qml index 8a24422779b4..f8f41c1c0f06 100644 --- a/gpt4all-chat/qml/ModelsView.qml +++ b/gpt4all-chat/qml/ModelsView.qml @@ -221,9 +221,7 @@ Rectangle { Layout.alignment: Qt.AlignTop | Qt.AlignHCenter visible: !isDownloading && (installed || isIncomplete) Accessible.description: qsTr("Remove model from filesystem") - onClicked: { - Download.removeModel(filename); - } + onClicked: Download.removeModel(id) } MySettingsButton { diff --git a/gpt4all-chat/src/download.cpp b/gpt4all-chat/src/download.cpp index 7d183768f131..799ad695932e 100644 --- a/gpt4all-chat/src/download.cpp +++ b/gpt4all-chat/src/download.cpp @@ -328,27 +328,28 @@ void Download::installCompatibleModel(const QString &modelName, const QString &a ModelList::globalInstance()->updateDataByFilename(modelFile, {{ ModelList::InstalledRole, true }}); } -void Download::removeModel(const QString &modelFile) +// FIXME(jared): With the current implementation, it is not possible to remove a duplicate +// model file (same filename, different subdirectory) from within GPT4All +// without restarting it. +void Download::removeModel(const QString &id) { auto *modelList = ModelList::globalInstance(); - auto *mySettings = MySettings::globalInstance(); - QFile incompleteFile(modelList->incompleteDownloadPath(modelFile)); - if (incompleteFile.exists()) - incompleteFile.remove(); + auto info = modelList->modelInfo(id); + if (info.id().isEmpty()) + return; - bool shouldRemoveInstalled = false; + Network::globalInstance()->trackEvent("remove_model", { {"model", info.filename()} }); - QFile file(mySettings->modelPath() + modelFile); - const ModelInfo info = modelList->modelInfoByFilename(modelFile); + // remove incomplete download + QFile(modelList->incompleteDownloadPath(info.filename())).remove(); - Network::globalInstance()->trackEvent("remove_model", { {"model", modelFile}, {"exists", file.exists()} }); + // remove file, if this is a real model + if (!info.isClone()) + QFile(info.path()).remove(); - if (file.exists()) { - modelList->uninstall(info); - if (!info.isClone()) - file.remove(); - } + // remove model list entry + modelList->uninstall(info); emit toastMessage(tr("Model \"%1\" is removed.").arg(info.name())); } diff --git a/gpt4all-chat/src/download.h b/gpt4all-chat/src/download.h index 9cb46a9e09a3..a4b1541ceed9 100644 --- a/gpt4all-chat/src/download.h +++ b/gpt4all-chat/src/download.h @@ -65,7 +65,7 @@ class Download : public QObject Q_INVOKABLE void cancelDownload(const QString &modelFile); Q_INVOKABLE void installModel(const QString &modelFile, const QString &apiKey); Q_INVOKABLE void installCompatibleModel(const QString &modelName, const QString &apiKey, const QString &baseUrl); - Q_INVOKABLE void removeModel(const QString &modelFile); + Q_INVOKABLE void removeModel(const QString &id); Q_INVOKABLE bool isFirstStart(bool writeVersion = false) const; public Q_SLOTS: From 1cbea1027fc86afce0c8d340efbbc4e14a595c74 Mon Sep 17 00:00:00 2001 From: Jared Van Bortel Date: Mon, 7 Oct 2024 14:33:05 -0400 Subject: [PATCH 14/15] modellist: fix models.json cache location - The filename must have a version number, or we will possibly load the cache for the wrong version of GPT4All. - The file should be stored in an appropriate folder for cache, not in the settings location. Signed-off-by: Jared Van Bortel --- gpt4all-chat/src/modellist.cpp | 70 +++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 27 deletions(-) diff --git a/gpt4all-chat/src/modellist.cpp b/gpt4all-chat/src/modellist.cpp index 9a39b4ba9400..79376f41b7a4 100644 --- a/gpt4all-chat/src/modellist.cpp +++ b/gpt4all-chat/src/modellist.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -44,6 +45,8 @@ using namespace Qt::Literals::StringLiterals; //#define USE_LOCAL_MODELSJSON +#define MODELS_JSON_VERSION "3" + static const QStringList FILENAME_BLACKLIST { u"gpt4all-nomic-embed-text-v1.rmodel"_s }; QString ModelInfo::id() const @@ -1382,15 +1385,32 @@ void ModelList::updateModelsFromDirectory() } } -#define MODELS_VERSION 3 +static QString modelsJsonFilename() +{ + return QStringLiteral("models" MODELS_JSON_VERSION ".json"); +} + +static std::optional modelsJsonCacheFile() +{ + constexpr auto loc = QStandardPaths::CacheLocation; + QString modelsJsonFname = modelsJsonFilename(); + if (auto path = QStandardPaths::locate(loc, modelsJsonFname); !path.isEmpty()) + return std::make_optional(path); + if (auto path = QStandardPaths::writableLocation(loc); !path.isEmpty()) + return std::make_optional(path); + return std::nullopt; +} void ModelList::updateModelsFromJson() { + QString modelsJsonFname = modelsJsonFilename(); + #if defined(USE_LOCAL_MODELSJSON) - QUrl jsonUrl("file://" + QDir::homePath() + u"/dev/large_language_models/gpt4all/gpt4all-chat/metadata/models%1.json"_s.arg(MODELS_VERSION)); + QUrl jsonUrl(u"file://%1/dev/large_language_models/gpt4all/gpt4all-chat/metadata/%2"_s.arg(QDir::homePath(), modelsJsonFname)); #else - QUrl jsonUrl(u"http://gpt4all.io/models/models%1.json"_s.arg(MODELS_VERSION)); + QUrl jsonUrl(u"http://gpt4all.io/models/%1"_s.arg(modelsJsonFname)); #endif + QNetworkRequest request(jsonUrl); QSslConfiguration conf = request.sslConfiguration(); conf.setPeerVerifyMode(QSslSocket::VerifyNone); @@ -1409,18 +1429,15 @@ void ModelList::updateModelsFromJson() qWarning() << "WARNING: Could not download models.json synchronously"; updateModelsFromJsonAsync(); - QSettings settings; - QFileInfo info(settings.fileName()); - QString dirPath = info.canonicalPath(); - const QString modelsConfig = dirPath + "/models.json"; - QFile file(modelsConfig); - if (!file.open(QIODeviceBase::ReadOnly)) { - qWarning() << "ERROR: Couldn't read models config file: " << modelsConfig; - } else { - QByteArray jsonData = file.readAll(); - file.close(); + auto cacheFile = modelsJsonCacheFile(); + if (!cacheFile) { + // no known location + } else if (cacheFile->open(QIODeviceBase::ReadOnly)) { + QByteArray jsonData = cacheFile->readAll(); + cacheFile->close(); parseModelsJsonFile(jsonData, false); - } + } else if (cacheFile->exists()) + qWarning() << "ERROR: Couldn't read models.json cache file: " << cacheFile->fileName(); } delete jsonReply; } @@ -1429,12 +1446,14 @@ void ModelList::updateModelsFromJsonAsync() { m_asyncModelRequestOngoing = true; emit asyncModelRequestOngoingChanged(); + QString modelsJsonFname = modelsJsonFilename(); #if defined(USE_LOCAL_MODELSJSON) - QUrl jsonUrl("file://" + QDir::homePath() + u"/dev/large_language_models/gpt4all/gpt4all-chat/metadata/models%1.json"_s.arg(MODELS_VERSION)); + QUrl jsonUrl(u"file://%1/dev/large_language_models/gpt4all/gpt4all-chat/metadata/%2"_s.arg(QDir::homePath(), modelsJsonFname)); #else - QUrl jsonUrl(u"http://gpt4all.io/models/models%1.json"_s.arg(MODELS_VERSION)); + QUrl jsonUrl(u"http://gpt4all.io/models/%1"_s.arg(modelsJsonFname)); #endif + QNetworkRequest request(jsonUrl); QSslConfiguration conf = request.sslConfiguration(); conf.setPeerVerifyMode(QSslSocket::VerifyNone); @@ -1497,17 +1516,14 @@ void ModelList::parseModelsJsonFile(const QByteArray &jsonData, bool save) } if (save) { - QSettings settings; - QFileInfo info(settings.fileName()); - QString dirPath = info.canonicalPath(); - const QString modelsConfig = dirPath + "/models.json"; - QFile file(modelsConfig); - if (!file.open(QIODeviceBase::WriteOnly)) { - qWarning() << "ERROR: Couldn't write models config file: " << modelsConfig; - } else { - file.write(jsonData); - file.close(); - } + auto cacheFile = modelsJsonCacheFile(); + if (!cacheFile) { + // no known location + } else if (QFileInfo(*cacheFile).dir().mkpath(u"."_s) && cacheFile->open(QIODeviceBase::WriteOnly)) { + cacheFile->write(jsonData); + cacheFile->close(); + } else + qWarning() << "ERROR: Couldn't write models config file: " << cacheFile->fileName(); } QJsonArray jsonArray = document.array(); From 4e8b82c71ddd3b53c6cc89eafa8328537a683fdd Mon Sep 17 00:00:00 2001 From: Jared Van Bortel Date: Mon, 7 Oct 2024 14:44:12 -0400 Subject: [PATCH 15/15] changelog: add this PR Signed-off-by: Jared Van Bortel --- gpt4all-chat/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/gpt4all-chat/CHANGELOG.md b/gpt4all-chat/CHANGELOG.md index 4a00708716f2..17f2c95b5a20 100644 --- a/gpt4all-chat/CHANGELOG.md +++ b/gpt4all-chat/CHANGELOG.md @@ -22,6 +22,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - Fix the local server rejecting min\_p/top\_p less than 1 ([#2996](https://github.com/nomic-ai/gpt4all/pull/2996)) - Fix "regenerate" always forgetting the most recent message ([#3011](https://github.com/nomic-ai/gpt4all/pull/3011)) - Fix loaded chats forgetting context when there is a system prompt ([#3015](https://github.com/nomic-ai/gpt4all/pull/3015)) +- Fix scroll reset upon download, model removal sometimes not working, and models.json cache location ([#3034](https://github.com/nomic-ai/gpt4all/pull/3034)) ## [3.3.1] - 2024-09-27 ([v3.3.y](https://github.com/nomic-ai/gpt4all/tree/v3.3.y))