Conversation
There was a problem hiding this comment.
Why are we changing it, UseClassification as a name for a type doesn't seem too clear for me
There was a problem hiding this comment.
Changes were made to avoid name conflicts with the new classes introduced in this PR.
| score: number; | ||
| } | ||
|
|
||
| interface ObjectDetectionModule { |
| <summary>Type definitions</summary> | ||
|
|
||
| ```typescript | ||
| interface StyleTransferModule { |
| import { ETInput, getTypeIdentifier } from '../../types/common'; | ||
|
|
||
| export class ExecutorchModule { | ||
| static async load(modelSource: string) { |
There was a problem hiding this comment.
can't we use load function from BaseModule here?
There was a problem hiding this comment.
@jakmro To do this let's craete BaseModule in src/modules/BaseModule.ts liek so:
export class BaseModule {
static module:
| _StyleTransferModule
| _ObjectDetectionModule
| _ClassificationModule
| _ETModule;
static async load(modelSource: string | number) {
if (!modelSource) return;
let path = typeof modelSource === 'number' ? Image.resolveAssetSource(modelSource).uri : modelSource;
try {
await this.module.loadModule(path as string);
} catch (e) {
throw new Error(getError(e));
}
}
static async forward(..._: any[]): Promise<any> {
throw new Error("Method 'myMethod()' must be implemented.");
}
}
and then just extend both ExecutorchModule and BaseCVModule.
The change in let path is necessary for typescript to correctly infer path type as string.
There was a problem hiding this comment.
Also I think we should use type ResourceSource like in LLMModule
| @@ -72,44 +72,44 @@ const StyleTransfer = StyleTransferSpec | |||
| ); | |||
|
|
|||
There was a problem hiding this comment.
I think we should keep it as it was if we ever want to create separate instances per call instead of treating each module as singleton.
| import { Image } from 'react-native'; | ||
| import { getError } from '../../Error'; | ||
|
|
||
| export class BaseModule { |
There was a problem hiding this comment.
| export class BaseModule { | |
| export class BaseCVModule { |
| } | ||
|
|
||
| try { | ||
| await _ETModule.loadModule(path); |
There was a problem hiding this comment.
| await _ETModule.loadModule(path); | |
| await this.module.loadModule(path); |
|
|
||
| try { | ||
| const numberArray = [...input] as number[]; | ||
| return await _ETModule.forward(numberArray, shape, inputType); |
There was a problem hiding this comment.
| return await _ETModule.forward(numberArray, shape, inputType); | |
| return await this.module.forward(numberArray, shape, inputType); |
| }: Props): StyleTransferModule => { | ||
| const [module, _] = useState(() => new _StyleTransferModule()); | ||
| export const useStyleTransfer = ({ modelSource }: Props): UseStyleTransfer => { | ||
| const [module, _] = useState(() => _StyleTransferModule); |
| } | ||
|
|
||
| interface ClassificationModule { | ||
| interface UseClassification { |
There was a problem hiding this comment.
let's remove this naming completely and move this as inline type below. This would solve naming issues as well as give end user more clarity of what the type actually is instead of it being a cryptic UseClassification
| } | ||
|
|
||
| interface ObjectDetectionModule { | ||
| interface UseObjectDetection { |
| } | ||
|
|
||
| interface StyleTransferModule { | ||
| interface UseStyleTransfer { |
| @@ -72,44 +72,44 @@ const StyleTransfer = StyleTransferSpec | |||
| ); | |||
|
|
|||
| class _ObjectDetectionModule { | |||
There was a problem hiding this comment.
Let's propagate return types from turbo modules specs like so:
| class _ObjectDetectionModule { | |
| class _ObjectDetectionModule { | |
| static async forward(input: string) : ReturnType<ObjectDetectionInterface["forward"]> { |
There was a problem hiding this comment.
import { Spec as ObjectDetectionInterface } from './NativeObjectDetection';
There was a problem hiding this comment.
Do this for all methods for other classes in this file too.
| let modelUrl = modelSource; | ||
| let tokenizerUrl = tokenizerSource; | ||
|
|
||
| if (typeof modelSource === 'number') { | ||
| modelUrl = Image.resolveAssetSource(modelSource).uri; | ||
| } | ||
|
|
||
| if (typeof tokenizerSource === 'number') { | ||
| tokenizerUrl = Image.resolveAssetSource(tokenizerSource).uri; | ||
| } | ||
|
|
||
| await LLM.loadLLM( | ||
| modelUrl as string, | ||
| tokenizerUrl as string, |
There was a problem hiding this comment.
| let modelUrl = modelSource; | |
| let tokenizerUrl = tokenizerSource; | |
| if (typeof modelSource === 'number') { | |
| modelUrl = Image.resolveAssetSource(modelSource).uri; | |
| } | |
| if (typeof tokenizerSource === 'number') { | |
| tokenizerUrl = Image.resolveAssetSource(tokenizerSource).uri; | |
| } | |
| await LLM.loadLLM( | |
| modelUrl as string, | |
| tokenizerUrl as string, | |
| let modelUrl = typeof modelSource === 'number' ? Image.resolveAssetSource(modelSource).uri : modelSource; | |
| let tokenizerUrl = typeof tokenizerSource === 'number' ? Image.resolveAssetSource(tokenizerSource).uri : tokenizerSource; | |
| await LLM.loadLLM( | |
| modelUrl, | |
| tokenizerUrl, |
This allows typescript to infer types correctly
## Description This PR introduces hookless API and restructures the `./src` directory. ### Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [x] 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 ### Checklist - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have updated the documentation accordingly - [x] My changes generate no new warnings
Description
This PR introduces hookless API and restructures the
./srcdirectory.Type of change
Tested on
Checklist