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.

Sign up for GitHub

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

Jump to bottom

[HF][fix] Allow kwargs in ModelParser.run() #882

Merged
merged 3 commits into from
Jan 11, 2024
Merged

[HF][fix] Allow kwargs in ModelParser.run() #882

merged 3 commits into from
Jan 11, 2024

Conversation

rossdanlm
Copy link
Contributor

@rossdanlm rossdanlm commented Jan 11, 2024 •
edited
Loading

[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

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

After

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

Stack created with Sapling. Best reviewed with ReviewStack.

[ez][fix][HF]Explicitly set hf_model to None if it's empty string
1f1421b 
Lol literally a 1-line fix.

I tested by removing the model name explicitly from HF model parsers to use the default, but this errored since we're setting it to "" instead of None.
<img width="610" alt="Screenshot 2024-01-11 at 03 02 00" src="https://github.com/lastmile-ai/aiconfig/assets/151060367/1a045dbb-ad1c-46db-ba1c-32b05d923484">


## Test Plan

Before
```
model_name=""
```
I got an error saying that no model "" exists, forgot to take screenshot but it was real pls believe me


After
```
model_name=None
No model was supplied, defaulted to sshleifer/distilbart-cnn-12-6 and revision a4f8f3e (https://huggingface.co/sshleifer/distilbart-cnn-12-6).
Using a pipeline without specifying a model name and revision in production is not recommended.
```

https://github.com/lastmile-ai/aiconfig/assets/151060367/db6b35d4-9e03-4ff5-b035-c77263978fd4
@rossdanlm rossdanlm mentioned this pull request Jan 11, 2024
Merged
@rossdanlm rossdanlm marked this pull request as ready for review January 11, 2024 10:04
@rossdanlm rossdanlm mentioned this pull request Jan 11, 2024
Merged
This was referenced Jan 11, 2024
Merged
Closed
rossdanlm and others added 2 commits January 11, 2024 05:55
@rossdanlm
Revert "[HF][fix] Use ParameterizedModelParser instead of ModelParser"
4a487a6 
Reverts #877

This caused a bug because image and audio inputs can't be parametrized. See #882 for a better fix instead
[HF][fix] Allow kwargs in ModelParser.run()
c18c039 
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 rossdanlm mentioned this pull request Jan 11, 2024
Merged
saqadri
saqadri reviewed Jan 11, 2024
View reviewed changes
Copy link
Contributor

@saqadri saqadri left a comment

Choose a reason for hiding this comment

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

We can fix on top but this is a critical API we are adding this to.

rossdanlm added a commit that referenced this pull request Jan 11, 2024
@rossdanlm
[ez][fix][HF]Explicitly set hf_model to None if it's empty string (#879)
f9e8469 
[ez][fix][HF]Explicitly set hf_model to None if it's empty string

Lol literally a 1-line fix.

I tested by removing the model name explicitly from HF model parsers to
use the default, but this errored since we're setting it to "" instead
of None.
<img width="610" alt="Screenshot 2024-01-11 at 03 02 00"
src="https://github.com/lastmile-ai/aiconfig/assets/151060367/1a045dbb-ad1c-46db-ba1c-32b05d923484">


## Test Plan

Before
```
model_name=""
```
I got an error saying that no model "" exists, forgot to take screenshot
but it was real pls believe me


After
```
model_name=None
No model was supplied, defaulted to sshleifer/distilbart-cnn-12-6 and revision a4f8f3e (https://huggingface.co/sshleifer/distilbart-cnn-12-6).
Using a pipeline without specifying a model name and revision in production is not recommended.
```


https://github.com/lastmile-ai/aiconfig/assets/151060367/db6b35d4-9e03-4ff5-b035-c77263978fd4

---
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with
[ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/879).
* #885
* #883
* #882
* #881
* __->__ #879
@rossdanlm
Copy link
Contributor Author

rossdanlm commented Jan 11, 2024 •
edited
Loading

We can fix on top but this is a critical API we are adding this to.

The problem is this currently breaks though so we need this. This is btw already a problem that we've flagged and have Sudhanshu working on it in #664 (I need to reply back to him)

@rossdanlm rossdanlm mentioned this pull request Jan 11, 2024
Closed
saqadri added a commit that referenced this pull request Jan 11, 2024
@saqadri
Revert "[HF][fix] Use ParameterizedModelParser instead of ModelParser" (
3a02cb3 
#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
Copy link
Contributor

saqadri commented Jan 11, 2024

I looked at the code and I understand what the issue has been -- until now all our model parsers have been ParameterizedModelParser instances -- that one accepts kwargs in run. It stands to reason that the base class should also accept kwargs. Your change makes things more consistent, so thanks for the fix! And we can improve on this further. Config.run also accepts kwargs, and we should ensure every derived model parser accepts them too.

@saqadri saqadri merged commit a0c04c1 into main Jan 11, 2024
2 checks passed
saqadri added a commit that referenced this pull request Jan 11, 2024
@saqadri
[HF][fix] Small nit fixes to ASR: Remove "model" from completion para…
b098ca1 
…ms, get_output_text in right format (#883)

[HF][fix] Small nit fixes to ASR: Remove "model" from completion params,
get_output_text in right format


Getting an error saying that "model" param is defined twice, which it is
because we're passing it through completion params and again through the
explicit model param. This along with the change in
#879 will ensure that the
right model of `None` is passed if the value is empty string.

I also fixed typo in the aiconfig.json file


## Test Plan

I also had to change the data field from `"./hi.mp3"` to
`"cookbooks/Gradio/hi.mp3"` becuase I can only run the config from the
high level `aiconfig` directory due to this line when running the
editor:
https://github.com/lastmile-ai/aiconfig/blob/b632094618c8f96b186b1849d0036e1313e02c5a/python/src/aiconfig/scripts/aiconfig_cli.py#L126

After


https://github.com/lastmile-ai/aiconfig/assets/151060367/7387706f-deea-414b-b429-732c78322615

---
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with
[ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/883).
* #885
* __->__ #883
* #882
* #881
* #879
saqadri added a commit that referenced this pull request Jan 11, 2024
@saqadri
Use facebook/mms-tts-eng instead of suno/bark in Gradio cookbook for …
7e7d6ea 
…Text2Speech (#885)

Use facebook/mms-tts-eng instead of suno/bark in Gradio cookbook for
Text2Speech

Bark is ~5GB and took my laptop 30mins to download for the first time.
The Facebook MMS
(https://huggingface.co/facebook/mms-tts-eng?fbclid=IwAR22U5VSaZ_V7QW4TJe77CM7f4fsTHjqCMjgLKV31wUl66ww1vXYSrEl-MA)
is only ~100MB which is way better and at least for cookbook I feel much
more worth it to quickly test things out. Yes the output quality is way
more memey and "voice AI" stereotype, but I feel the tradeoff is worth
it

---
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with
[ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/885).
* __->__ #885
* #883
* #882
* #881
* #879
@rossdanlm rossdanlm deleted the pr882 branch January 12, 2024 18:56
@rossdanlm rossdanlm mentioned this pull request Feb 6, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saqadri saqadri saqadri approved these changes

@rholinshead rholinshead Awaiting requested review from rholinshead rholinshead is a code owner

@suyoglastmileai suyoglastmileai Awaiting requested review from suyoglastmileai

@Ankush-lastmile Ankush-lastmile Awaiting requested review from Ankush-lastmile Ankush-lastmile is a code owner

@jonathanlastmileai jonathanlastmileai Awaiting requested review from jonathanlastmileai jonathanlastmileai is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rossdanlm @saqadri