Skip to content

fix: issue with isModelGenerating when switching between multiple models#73

Merged
NorbertKlockiewicz merged 3 commits intomainfrom
@norbertklockiewicz/fix-isGenerating-issue
Dec 20, 2024
Merged

fix: issue with isModelGenerating when switching between multiple models#73
NorbertKlockiewicz merged 3 commits intomainfrom
@norbertklockiewicz/fix-isGenerating-issue

Conversation

@NorbertKlockiewicz
Copy link
Copy Markdown
Contributor

@NorbertKlockiewicz NorbertKlockiewicz commented Dec 19, 2024

Description

There was a problem when user was switching between multiple llms in one component, to fix this issue I removed code related to handling strict mode problems(strict mode was causing problems with event listeners also so I think we should ignore it as it isn't the best with background tasks), I've also remove deleteModule native function as it wasn't really doing anything, now user can download multiple llms within one component and seamlessly switch between them without bugs.
The problem was mentioned in issue #42

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update (improves or adds clarity to existing documentation)

Tested on

  • iOS
  • Android

Testing instructions

Screenshots

Related issues

Checklist

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly
  • My changes generate no new warnings

Additional notes

if (llamaModule != null || isFetching) {
promise.reject("Model already loaded", "Model is already loaded or fetching")
if (isFetching) {
promise.reject("Model already loaded", "Model is already fetching")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first argument seems wrong

Comment thread ios/RnExecutorch/LLM.mm Outdated
if(self->runner || isFetching){

if(isFetching){
reject(@"model_already_loaded", @"Model and tokenizer already loaded", nil);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this error message correct? you're not checking if its loaded

llamaModule!!.stop()
}

override fun deleteModule() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we getting rid of this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that it doesn't really change memory usage, but let's leave it for now as it's not breaking anything

@NorbertKlockiewicz NorbertKlockiewicz merged commit cf7f372 into main Dec 20, 2024
@NorbertKlockiewicz NorbertKlockiewicz deleted the @norbertklockiewicz/fix-isGenerating-issue branch December 20, 2024 16:39
mkopcins pushed a commit that referenced this pull request Oct 15, 2025
…els (#73)

## Description
There was a problem when user was switching between multiple llms in one
component, to fix this issue I removed code related to handling strict
mode problems(strict mode was causing problems with event listeners also
so I think we should ignore it as it isn't the best with background
tasks), I've also remove deleteModule native function as it wasn't
really doing anything, now user can download multiple llms within one
component and seamlessly switch between them without bugs.
The problem was mentioned in issue #42 

### Type of change
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] Documentation update (improves or adds clarity to existing
documentation)

### Tested on
- [x] iOS
- [x] Android

### Testing instructions
<!-- Provide step-by-step instructions on how to test your changes.
Include setup details if necessary. -->

### Screenshots
<!-- Add screenshots here, if applicable -->

### Related issues
<!-- Link related issues here using #issue-number -->

### Checklist
- [x] I have performed a self-review of my code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have updated the documentation accordingly
- [ ] My changes generate no new warnings

### Additional notes
<!-- Include any additional information, assumptions, or context that
reviewers might need to understand this PR. -->
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