Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

modellist: work around filtered item models getting out of sync #2545

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

cebtenzzre
Copy link
Member

@cebtenzzre cebtenzzre commented Jul 3, 2024

I have yet to understand exactly why, but since #2086 the "Clone" button on the models page sometimes results in the entire settings page being replaced with blank values instead of a cloned model.

I now know that when the "Clone" button is clicked, the id returned fromModelList::clone is not found in the combo box for selection, which leads to index=-1 -> selected value=undefined -> default-constructed ModelInfo() when values are accessed.

This stems from the fact that m_selectableModels doesn't contain the newly added model by the end of ModelList::clone. For whatever reason, the initial ModelList::addModel call does fire an InstalledModels::filterAcceptsRow check on the cloned model, but the ModelList::updateModel call to set its data (which does call ModelList::dataChanged) does not. So m_selectableModels incorrectly thinks the cloned model is not yet "installed."

This PR works around that by explicitly invalidating all of the filter models at the end of ModelList::updateData. In the future, we should figure out why the dataChanged signal does not result in the expected update of the child models.


🚀 This description was created by Ellipsis for commit f3f598a

Summary:

Fixes issue with 'Clone' button resulting in blank values by invalidating filter models in ModelList::updateData.

Key points:

  • Issue: 'Clone' button on models page sometimes results in blank values on settings page.
  • Root Cause: m_selectableModels does not contain the newly added model by the end of ModelList::clone.
  • Fix: Explicitly invalidate filter models (m_selectableModels, m_installedModels, m_downloadableModels) at the end of ModelList::updateData.
  • Files Affected: gpt4all-chat/modellist.cpp.
  • Functions Affected: ModelList::updateData.

Generated with ❤️ by ellipsis.dev

Signed-off-by: Jared Van Bortel <jared@nomic.ai>
@cebtenzzre cebtenzzre requested a review from manyoso July 3, 2024 20:59
ellipsis-dev[bot]

This comment was marked as spam.

@cebtenzzre cebtenzzre merged commit 30692a2 into main Jul 3, 2024
6 of 12 checks passed
manyoso added a commit that referenced this pull request Sep 20, 2024
…nc (#2545)"

This is what caused regression seen in issue #2943

This reverts commit 30692a2.
manyoso added a commit that referenced this pull request Oct 3, 2024
…nc (#2545)"

This is what caused regression seen in issue #2943

This reverts commit 30692a2.

Signed-off-by: Adam Treat <treat.adam@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants