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

[HF][fix] Use ParameterizedModelParser instead of ModelParser #877

Merged
merged 1 commit into from
Jan 11, 2024
Merged

Conversation

rossdanlm
Copy link
Contributor

@rossdanlm rossdanlm commented Jan 11, 2024

[HF][fix] Use ParameterizedModelParser instead of ModelParser

Fixing this error:

Exception: HuggingFaceImage2TextTransformer.run() got an unexpected keyword argument 'callback_manager'

This seems like a pretty brittle bug. If we use ModelParser isntead of ParameterizedModelParser, we get a bug because callback_manager is not a supported kwargs for the run_inference() method. This gets filtered out by the ParameterizedModelParser.run() method (

**kwargs,
) -> List[Output]:
# maybe use prompt metadata instead of kwargs?
if kwargs.get("run_with_dependencies", False):
return await self.run_with_dependencies(prompt, aiconfig, options, parameters)
else:
return await self.run_inference(prompt, aiconfig, options, parameters)
) but these kwags are not accepted by the ModelParser.run() method (
@abstractmethod
async def run(
self,
prompt: Prompt,
aiconfig: AIConfig,
options: Optional["InferenceOptions"] = None,
parameters: Dict = {},
) -> ExecuteResult:
)

Test Plan

Before

Screen.Recording.2024-01-11.at.02.00.14.mov

After

Screen.Recording.2024-01-11.at.01.59.36.mov

rossdanlm added a commit that referenced this pull request Jan 11, 2024
Add model setting completion params for Image2Text prompt schema


TSIA, pretty simple actually

## Test Plan
Follow the README from AIConfig Editor
https://github.com/lastmile-ai/aiconfig/tree/main/python/src/aiconfig/editor#dev,
then run these commands:

Make sure also that you don't have the `python-aiconfig-test` installed:
`pip3 uninstall python-aiconfig-test`

```bash
aiconfig_path=/Users/rossdancraig/Projects/aiconfig/cookbooks/Gradio/huggingface.aiconfig.json
parsers_path=/Users/rossdancraig/Projects/aiconfig/cookbooks/Gradio/hf_model_parsers.py
alias aiconfig="python3 -m 'aiconfig.scripts.aiconfig_cli'"
aiconfig edit --aiconfig-path=$aiconfig_path --server-port=8080 --server-mode=debug_servers --parsers-module-path=$parsers_path
```

<img width="1261" alt="Screenshot 2024-01-11 at 01 21 21"
src="https://github.com/lastmile-ai/aiconfig/assets/151060367/63ef4830-1163-4229-90ea-d49b914d1ec2">

---
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with
[ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/875).
* #877
* __->__ #875
@rossdanlm rossdanlm changed the title [fix] Use ParameterizedModelParser instead of ModelParser [HF][fix] Use ParameterizedModelParser instead of ModelParser Jan 11, 2024
Fixing this error:
```
Exception: HuggingFaceImage2TextTransformer.run() got an unexpected keyword argument 'callback_manager'
```

This seems like a pretty brittle bug. If we use `ModelParser` isntead of `ParameterizedModelParser`, we get a bug because `callback_manager` is not a supported kwargs for the `run_inference()` method. This gets filtered out by the `ParameterizedModelParser.run()` method (https://github.com/lastmile-ai/aiconfig/blob/0ceb17636bae2b5416e7b415cdc87fd71b5ba3b0/python/src/aiconfig/default_parsers/parameterized_model_parser.py#L48-L54) but these kwags are not accepted  by the `ModelParser.run()` method (https://github.com/lastmile-ai/aiconfig/blob/0ceb17636bae2b5416e7b415cdc87fd71b5ba3b0/python/src/aiconfig/model_parser.py#L62-L69)

## Test Plan

Before

https://github.com/lastmile-ai/aiconfig/assets/151060367/a23a5d5c-d9a2-415b-8a6e-9826da56e985


After

https://github.com/lastmile-ai/aiconfig/assets/151060367/8899c527-7496-410b-b6b6-9e5a9525b1b5
@rossdanlm rossdanlm marked this pull request as ready for review January 11, 2024 07:11
@rossdanlm rossdanlm merged commit 055fc33 into main Jan 11, 2024
@rossdanlm rossdanlm deleted the pr877 branch January 11, 2024 07:29
rossdanlm pushed a commit that referenced this pull request Jan 11, 2024
Getting this error:
```
Exception: Cannot get prompt template string from prompt input: attachments=[Attachment(data='https://s3.amazonaws.com/lastmileai.aiconfig.public/uploads/2024_1_10_18_41_31/1742/bear_with_honey.png', mime_type='image/png', metadata=None), Attachment(data='https://s3.amazonaws.com/lastmileai.aiconfig.public/uploads/2024_1_10_18_41_31/7275/fox_in_forest.png', mime_type='image/png', metadata=None)] data=None
```

When Sarmad did this fix in #866, he missed a callsite in the `parameterized_model_parser`. This was missed because at the time the image_2_text model parser was of type `ModelParser` instead of `ParameterizedModelParser`, which had to be converted in here to fix a bug: #877

Before
rossdanlm pushed a commit that referenced this pull request Jan 11, 2024
Getting this error:
```
Exception: Cannot get prompt template string from prompt input: attachments=[Attachment(data='https://s3.amazonaws.com/lastmileai.aiconfig.public/uploads/2024_1_10_18_41_31/1742/bear_with_honey.png', mime_type='image/png', metadata=None), Attachment(data='https://s3.amazonaws.com/lastmileai.aiconfig.public/uploads/2024_1_10_18_41_31/7275/fox_in_forest.png', mime_type='image/png', metadata=None)] data=None
```

When Sarmad did this fix in #866, he missed a callsite in the `parameterized_model_parser`. This was missed because at the time the image_2_text model parser was of type `ModelParser` instead of `ParameterizedModelParser`, which had to be converted in here to fix a bug: #877


## Test Plan

Before
<img width="1295" alt="Screenshot 2024-01-11 at 02 52 42" src="https://github.com/lastmile-ai/aiconfig/assets/151060367/c0c99f1e-2996-41f3-8274-c13ecc43f07e">


After

https://github.com/lastmile-ai/aiconfig/assets/151060367/7a10932c-8580-4584-8942-4c287f23fc82
rossdanlm pushed a commit that referenced this pull request Jan 11, 2024
See #877 for context, and #880 for why that original fix needed to be reverted

Just a quick hack to get unblocked. I tried originally to make the ASR and Image2Text ParameterizedModel but that caused other errors.

## Test Plan

Before

https://github.com/lastmile-ai/aiconfig/assets/151060367/a23a5d5c-d9a2-415b-8a6e-9826da56e985
rossdanlm pushed a commit that referenced this pull request Jan 11, 2024
See #877 for context, and #880 for why that original fix needed to be reverted

Just a quick hack to get unblocked. I tried originally to make the ASR and Image2Text ParameterizedModel but that caused other errors.

## Test Plan

Before

https://github.com/lastmile-ai/aiconfig/assets/151060367/a23a5d5c-d9a2-415b-8a6e-9826da56e985

After

https://github.com/lastmile-ai/aiconfig/assets/151060367/f29580e9-5cf6-43c5-b848-bb525eb368f2
rossdanlm pushed a commit that referenced this pull request Jan 11, 2024
See #877 for context, and #880 for why that original fix needed to be reverted

Just a quick hack to get unblocked. I tried originally to make the ASR and Image2Text ParameterizedModel but that caused other errors.

## Test Plan

Before

https://github.com/lastmile-ai/aiconfig/assets/151060367/a23a5d5c-d9a2-415b-8a6e-9826da56e985

After

https://github.com/lastmile-ai/aiconfig/assets/151060367/f29580e9-5cf6-43c5-b848-bb525eb368f2
rossdanlm added a commit that referenced this pull request Jan 11, 2024
Reverts #877

This caused a bug because image and audio inputs can't be parametrized. See #882 for a better fix instead
rossdanlm pushed a commit that referenced this pull request Jan 11, 2024
See #877 for context, and #880 for why that original fix needed to be reverted

Just a quick hack to get unblocked. I tried originally to make the ASR and Image2Text ParameterizedModel but that caused other errors.

## Test Plan

Before

https://github.com/lastmile-ai/aiconfig/assets/151060367/a23a5d5c-d9a2-415b-8a6e-9826da56e985

After

https://github.com/lastmile-ai/aiconfig/assets/151060367/f29580e9-5cf6-43c5-b848-bb525eb368f2
@saqadri
Copy link
Contributor

saqadri commented Jan 11, 2024

Will leave a comment in other diff as well, but it's important that models that can't have paramrterized inputs NOT extend that ParameterizedModelParser class

saqadri added a commit that referenced this pull request Jan 11, 2024
#881)

Revert "[HF][fix] Use ParameterizedModelParser instead of ModelParser"

Reverts #877

This caused a bug because image and audio inputs can't be parametrized.
See #882 for a better fix
instead

---
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with
[ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/881).
* #885
* #883
* #882
* __->__ #881
* #879
saqadri added a commit that referenced this pull request Jan 11, 2024
[HF][fix] Allow `kwargs` in `ModelParser.run()`



See #877 for context, and
#880 for why that original
fix needed to be reverted

Just a quick hack to get unblocked. I tried originally to make the ASR
and Image2Text ParameterizedModel but that caused other errors.

## Test Plan

Before


https://github.com/lastmile-ai/aiconfig/assets/151060367/a23a5d5c-d9a2-415b-8a6e-9826da56e985

After


https://github.com/lastmile-ai/aiconfig/assets/151060367/f29580e9-5cf6-43c5-b848-bb525eb368f2

---
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with
[ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/882).
* #885
* #883
* __->__ #882
* #881
* #879
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