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

Updated ParameterizedModelParser run method for handing run_with_dependencies parameter #923

Merged
merged 28 commits into from
Mar 27, 2024

Conversation

sp6370
Copy link
Contributor

@sp6370 sp6370 commented Jan 14, 2024

Context: Currently ParameterizedModelParser run method utilized kwargs to decide if to execute prompt with dependencies or not. This pull request eliminates the usage of kwargs and introduces a new optional prameter for run method to pass information about running the prompt with dependencies.

Closes: #664

Test Plan:

  • Add a new test that invokes run with run_with_dependencies=True for a prompt which requires output from another prompt.
  • Ensure that all the prompts in the hugging face cookbook work.

@sp6370
Copy link
Contributor Author

sp6370 commented Jan 14, 2024

Hi @rossdanlm!

Could I get some feedback on the test for the PR?

@sp6370 sp6370 marked this pull request as ready for review January 27, 2024 21:39
@sp6370 sp6370 changed the title [WIP] Updated ParameterizedModelParser run method for handing run_with_dependencies parameter Updated ParameterizedModelParser run method for handing run_with_dependencies parameter Jan 27, 2024
Copy link
Contributor

@jonathanlastmileai jonathanlastmileai left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Back to your queue for now

@@ -48,3 +50,23 @@ async def test_load_parametrized_data_config(set_temporary_env_vars):
},
],
}

@pytest.mark.xfail
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we adding a test we expect to fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned by @Ankush-lastmile here. There have been changes to OpenAI model paeser as such new way to for Mocking OpenAI API is required.

The new test introduced works fines based on the earlier mocking approach. Unless a mocking strategy is implemented we would have to use the work around as used here as well https://github.com/sp6370/aiconfig/blob/9617bd9d54e3bff3bb7eef2889af9f30adf39602/python/tests/test_run_config.py#L11.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's good to add, we can edit it later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have e-enabled the test.

Copy link
Contributor

@rossdanlm rossdanlm left a comment

Choose a reason for hiding this comment

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

I think this is good, thanks for the fix, pls ping me when this lands!

@@ -48,3 +50,23 @@ async def test_load_parametrized_data_config(set_temporary_env_vars):
},
],
}

@pytest.mark.xfail
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's good to add, we can edit it later

Copy link
Contributor

@rossdanlm rossdanlm left a comment

Choose a reason for hiding this comment

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

Actually wait sorry, can you pls also remove kwargs from

**kwargs, # TODO: Remove this, just a hack for now to ensure that it doesn't break
?

See more comments and details in #882

@sp6370
Copy link
Contributor Author

sp6370 commented Feb 11, 2024

Actually wait sorry, can you pls also remove kwargs from

**kwargs, # TODO: Remove this, just a hack for now to ensure that it doesn't break

?
See more comments and details in #882

Hi @rossdanlm!
I have addressed this. Also, here's a short video for testing Hugging Face Functionality.
https://www.loom.com/share/a7e09d403dcd4d30a7fd93f8a9e8b7c5

This is GitHub issue for ffmpeg missing error.
#1154

Copy link
Contributor

@rossdanlm rossdanlm left a comment

Choose a reason for hiding this comment

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

Pls ensure that when testing it's linked to the local pacakge

for input_params, response in response_map_list:
if kwargs == input_params:
return response
raise Exception("Unexpected arguments:\n {}".format(kwargs))
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably shoudl remove now to show that it works now that kwargs is removed

Copy link
Contributor

@rossdanlm rossdanlm left a comment

Choose a reason for hiding this comment

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

Generally lgtm, just make sure that we've chcked all the default model parsers too:

aiconfig/python/src/aiconfig/default_parsers/.*

@sp6370
Copy link
Contributor Author

sp6370 commented Mar 20, 2024

Cross checked the default model parsers aiconfig/python/src/aiconfig/default_parsers/.*. They don't have any implementations of run which requires update.

new=mock_openai,
):
config_relative_path = (
"aiconfigs/tarvel_gpt_prompts_with_dependency.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: travel typo

Copy link
Contributor

@rossdanlm rossdanlm left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for the fixes! I'll follow up later with extensions + publishing requirements!

@rossdanlm rossdanlm dismissed jonathanlastmileai’s stale review March 20, 2024 21:56

This is from a while ago, dismissing to land this

@sp6370
Copy link
Contributor Author

sp6370 commented Mar 21, 2024

@rossdancraig I share the info on the test plan later today.

@sp6370
Copy link
Contributor Author

sp6370 commented Mar 22, 2024

Tests:

@rossdanlm
Copy link
Contributor

@rossdanlm rossdanlm merged commit a11d07a into lastmile-ai:main Mar 27, 2024
2 checks passed
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.

[python] Add run_with_dependencies to API
5 participants