Add list-suts command (#257) #263

0xkerem · 2024-03-09T13:59:51Z

This pull request encompasses the addition of the 'list-suts' command as outlined in issue #257, along with the necessary test for this new feature. The key activities included are as follows:

list-suts Command Implementation: A new command has been introduced to list valid values for the --sut option. This command also indicates whether a SUT requires secrets.
Testing Process: Tests pertinent to the newly added command have been written and executed successfully. These tests verify that the command functions as expected.
Documentation Update: Information on how to use the newly added 'list-suts' command has been incorporated into the project documentation.

Here are terminal output examples showcasing the list-suts command in action: one displaying a SUT with its required secrets, and the other indicating a SUT with missing secrets.

$ poetry run python newhelm/main.py list-suts

gpt2
class=HuggingFaceSUT
args=['gpt2', SerializedSecret(module='newhelm.suts.huggingface_client', qual_name='HuggingFaceToken')]
kwargs={}
Uses required secrets:
{'demo': {'demo_api_key': '12345'}}

Cannot display SUT llama-2-7b because it requires the following secrets:
scope='together' key='api_key' instructions='See https://api.together.xyz/settings/api-keys'

* Implemented the list-suts command in newhelm_cli * Added functionality to display each SUT with required secrets * Refactored some parts of the display logic for better clarity * This is the base version of the function, it will be imprpoved

* secret_values.py: InjectSecret.__repr__ initialized * main.py: list_suts output formatted for SUTs with required secrets

* Updated list_suts for desired output. * Updated dev_quick_start.md to include new list-suts command. * Restored secret_values.py (No need for this issue)

- Add test_list_suts to test_cli.py - Use @expensive_tests decorator for conditional skipping - Confirm test passes when run without the @expensive_tests

github-actions · 2024-03-09T14:00:02Z

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

0xkerem · 2024-03-09T14:03:38Z

Hello @brianwgoldman , I read CONTRIBUTING.md before PR, but I could not access the form specified in the link.

mkly · 2024-03-09T15:44:30Z

Hi @0xkerem ! Very excited to see your interest in contributing. Are you able to access https://mlcommons.org/community/subscribe/ ?

If so, you can add your name, email address, and Github ID to that form? You should receive an email with with a document to fill out for the CLA. If you do not receive a response to that within a day or two please let me know here. Thanks.

0xkerem · 2024-03-09T15:53:51Z

Hi @0xkerem ! Very excited to see your interest in contributing. Are you able to access https://mlcommons.org/community/subscribe/ ?

@mkly Unfortunately, this page does not open, I encounter the error notification shown in the screenshot.

mkly · 2024-03-09T16:18:45Z

Thanks for that additional info. I would go ahead and send an email to support@mlcommons.org with your full name and Github ID. I would also link to this thread and include that you are looking to sign the ML Commons Contributor License Agreement(CLA). Sorry for the trouble!

0xkerem · 2024-03-09T17:03:53Z

Thank you so much for your help. No problem :)

0xkerem · 2024-03-11T10:52:00Z

@mkly Hi! Have you received any answers?

brianwgoldman · 2024-03-11T14:11:53Z

newhelm/main.py

+        click.echo(f"args={sut_obj._initialization_record.args}") # Consider create getter function for `_initialization_record`
+        click.echo(f"kwargs={sut_obj._initialization_record.kwargs}")
+        click.echo(f"Uses required secrets:")
+        click.echo(secrets)


secrets here is everything you defined in your config/secrets.toml file, and will be the same for all SUTs.

In order to get this behavior I think we are going to need to add functionality to dependency_injection that tells us what dependencies were used.

brianwgoldman · 2024-03-11T14:12:45Z

newhelm/main.py

+
+        display_header(sut)
+        click.echo(f"class={sut_obj.__class__.__name__}")
+        click.echo(f"args={sut_obj._initialization_record.args}") # Consider create getter function for `_initialization_record`


The sut_entry variable in this loop is a FactoryEntry which has all the args and kwargs in it already. You can get this information without attempting to construct the sut_obj.

brianwgoldman · 2024-03-11T14:14:05Z

newhelm/main.py

+
+        display_header(sut)
+        click.echo(f"class={sut_obj.__class__.__name__}")
+        click.echo(f"args={sut_obj._initialization_record.args}") # Consider create getter function for `_initialization_record`


Consider create getter function for _initialization_record

You can use get_initialization_record, although as mentioned in my other comment getting this data directly from FactorEntry is probably cleaner.

@brianwgoldman Hello, you explained it very well again, thank you for this and for your feedback. I tried to understand and apply it as accurately as I could. If I make a mistake again, I'm waiting for your feedback!

Sample printouts for missing secret:

$ poetry run python newhelm/main.py list-suts llama-2-7b class=TogetherCompletionsSUT args=('togethercomputer/llama-2-7b', <newhelm.secret_values.InjectSecret object at 0x0000021F76C56740>) kwargs={} Missing required secrets: scope='together' key='api_key', instructions='See https://api.together.xyz/settings/api-keys' gpt-3.5-turbo class=OpenAIChat args=('gpt-3.5-turbo', <newhelm.secret_values.InjectSecret object at 0x0000021F772FA3B0>, <newhelm.secret_values.InjectSecret object at 0x0000021F772FAF20>) kwargs={} Missing required secrets: scope='openai' key='api_key', instructions='See https://platform.openai.com/api-keys'

Sample outputs when the required secret conditions are met:

$ poetry run python newhelm/main.py list-suts demo_yes_no class=DemoYesNoSUT args=() kwargs={} Uses required secrets: [] gpt2 class=HuggingFaceSUT args=('gpt2', <newhelm.secret_values.InjectSecret object at 0x0000021F0D09BCD0>) kwargs={} Uses required secrets: [HuggingFaceToken()] gpt-3.5-turbo class=OpenAIChat args=('gpt-3.5-turbo', <newhelm.secret_values.InjectSecret object at 0x000002168868A350>, <newhelm.secret_values.InjectSecret object at 0x000002168868AEC0>) kwargs={} Uses required secrets: [OpenAIApiKey(), OpenAIOrgId()]

mkly · 2024-03-11T17:06:53Z

@0xkerem I should have mentioned that it might take a few days for them to get back to you. I'll follow up tomorrow if you still have not received anything. Thanks for your patience.

* Improved readability and structure of list_suts. * Updated inject_dependencies to handle new secrets. * Enhanced inject_dependencies with three return values. * Modified all occurrences of inject_dependencies to align with its updated usage.

brianwgoldman · 2024-03-13T15:36:09Z

newhelm/dependency_injection.py

@@ -11,28 +11,40 @@

 def inject_dependencies(


I think the modifications to this method make the most common use case (setting dependencies) unnecessarily complicated to support a rare use case (check dependencies).

I think we should keep the signature of this method, and instead create a peer that is something like list_dependency_usage which returns:

All dependencies that were successfully read.

All dependencies that were required but not provided.

(Maybe) all dependencies that were optional but not provided.

Sorry for potentially misleading you with my previous comment.

brianwgoldman · 2024-03-13T15:37:09Z

newhelm/main.py

+        try:
+            _, _, used_secrets = inject_dependencies(sut_entry.args, sut_entry.kwargs, secrets)
+
+            flag = "Uses"


Ideally if a SUT had one secret provide and one missing, we could report both of those at the same time.

@brianwgoldman No problem! I'm glad to contribute code improvements. :) I've restored the inject_dependencies function and introduced list_dependency_usage. Currently, it doesn't return optional secrets, but but I will add it when I have time soon. Also, I've updated the formatting for better clarity: now, if there are both used and missing requirements, they will be displayed simultaneously. If there are no missing requirements, they won't be shown. Additionally, in cases where no secrets are needed, the message No Secrets Used. will be displayed.

Example for sut without used secrets and with missing secrets:

$ poetry run python newhelm/main.py list-suts llama-2-7b Class: TogetherCompletionsSUT Args: ('togethercomputer/llama-2-7b', <newhelm.secret_values.InjectSecret object at 0x00000228F06DBCD0>) Kwargs: {} No Secrets Used. Missing Secrets: Scope: 'together', Key: 'api_key', Instructions: 'See https://api.together.xyz/settings/api-keys'

Example for sut with used secrets and without missing secrets:

$ poetry run python newhelm/main.py list-suts gpt2 Class: HuggingFaceSUT Args: ('gpt2', <newhelm.secret_values.InjectSecret object at 0x0000022886B9BA30>) Kwargs: {} Used Secrets: [HuggingFaceToken()]

Example for sut without both used and missing secrets:

$ poetry run python newhelm/main.py list-suts Class: DemoYesNoSUT Args: () Kwargs: {} No Secrets Used.

Example for sut with both used and missing secrets:

$ poetry run python newhelm/main.py list-suts gpt-3.5-turbo Class: OpenAIChat Args: ('gpt-3.5-turbo', <newhelm.secret_values.InjectSecret object at 0x00000228F0D7A110>, <newhelm.secret_values.InjectSecret object at 0x00000228F0D7AC80>) Kwargs: {} Used Secrets: [OpenAIOrgId()] Missing Secrets: Scope: 'openai', Key: 'api_key', Instructions: 'See https://platform.openai.com/api-keys'

I'm sorry that I accidentally used the name of the internal function in the commit message because I was distracted.

* Reverted modifications in inject_dependencies to original state. * Introduced format_missing_secrets function. * Corrected usage of inject_dependencies across codebase. * Improved formatting and clarity in list_suts function.

nathanw-mlc · 2024-03-14T19:30:22Z

Unfortunately, this page does not open, I encounter the error notification shown in the screenshot.

@0xkerem are you using a DNS ad-blocker of some kind? I've found that some lists block Mailchimp domains.

0xkerem · 2024-03-14T19:46:09Z

@0xkerem are you using a DNS ad-blocker of some kind? I've found that some lists block Mailchimp domains.

@nathanw-mlc I use regular ad blocker but when I disabled it, it didn't work. Then I tried disabling the antivirus and using another browser, but still the same error. I thought connecting via VPN might help, this time I can open the form, I will fill it out now. I hope it's okay to use vpn to fill out the form.

nathanw-mlc · 2024-03-14T19:52:55Z

I hope it's okay to use vpn to fill out the form.

So long as the VPN provider doesn't have DNS rules that block the form, there's absolutely no problem with using a VPN.

0xkerem · 2024-03-14T20:44:16Z

@mkly and @nathanw-mlc Thank you very much for your help. I filled out the form and digitally filled out and signed the agreement in the email I received. Then I sent it to the specified email.

0xkerem · 2024-03-14T21:29:10Z

recheck

mkly · 2024-03-19T17:52:43Z

@0xkerem Looks like there are a few formatting issues with the linter. You can run the following in the root of the repo to fix those.

poetry run black .

After that, you should be green to merge this in. Thanks again!

…to cli-list-suts

0xkerem · 2024-03-20T12:13:52Z

@mkly Thank you! I fixed that and the remaining errors. I guess there is no problem anymore.

0xkerem added 5 commits March 9, 2024 01:37

Updated list_suts in CLI (mlcommons#257)

7b7f9b0

* secret_values.py: InjectSecret.__repr__ initialized * main.py: list_suts output formatted for SUTs with required secrets

Updated list_suts and dev_quick_start

b8598c6

* Updated list_suts for desired output. * Updated dev_quick_start.md to include new list-suts command. * Restored secret_values.py (No need for this issue)

Refine list-suts docs in dev_quick_start.md (mlcommons#257)

65c2fcc

Add list-suts command test (mlcommons#257)

dc5d87e

- Add test_list_suts to test_cli.py - Use @expensive_tests decorator for conditional skipping - Confirm test passes when run without the @expensive_tests

brianwgoldman suggested changes Mar 11, 2024

View reviewed changes

brianwgoldman reviewed Mar 13, 2024

View reviewed changes

brianwgoldman approved these changes Mar 13, 2024

View reviewed changes

0xkerem and others added 2 commits March 13, 2024 23:43

Create format_missing_secrets function. (mlcommons#257)

7b41339

* Reverted modifications in inject_dependencies to original state. * Introduced format_missing_secrets function. * Corrected usage of inject_dependencies across codebase. * Improved formatting and clarity in list_suts function.

Merge branch 'mlcommons:main' into cli-list-suts

52f0467

0xkerem added 3 commits March 20, 2024 14:49

Files reformatted (mlcommons#257)

666ff4b

Merge branch 'cli-list-suts' of https://github.com/0xkerem/newhelm in…

173a088

…to cli-list-suts

Add type hints in list_dependency_usage. (mlcommons#257)

aaa75c2

mkly merged commit f78627f into mlcommons:main Mar 20, 2024
2 checks passed

github-actions bot locked and limited conversation to collaborators Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add list-suts command (#257) #263

Add list-suts command (#257) #263

0xkerem commented Mar 9, 2024

github-actions bot commented Mar 9, 2024 •

edited

Loading

0xkerem commented Mar 9, 2024

mkly commented Mar 9, 2024

0xkerem commented Mar 9, 2024

mkly commented Mar 9, 2024

0xkerem commented Mar 9, 2024

0xkerem commented Mar 11, 2024 •

edited

Loading

brianwgoldman Mar 11, 2024

brianwgoldman Mar 11, 2024

brianwgoldman Mar 11, 2024

0xkerem Mar 12, 2024

mkly commented Mar 11, 2024

brianwgoldman Mar 13, 2024

brianwgoldman Mar 13, 2024

0xkerem Mar 13, 2024

nathanw-mlc commented Mar 14, 2024 •

edited

Loading

0xkerem commented Mar 14, 2024

nathanw-mlc commented Mar 14, 2024

0xkerem commented Mar 14, 2024

0xkerem commented Mar 14, 2024

mkly commented Mar 19, 2024

0xkerem commented Mar 20, 2024

Add list-suts command (#257) #263

Add list-suts command (#257) #263

Conversation

0xkerem commented Mar 9, 2024

github-actions bot commented Mar 9, 2024 • edited Loading

0xkerem commented Mar 9, 2024

mkly commented Mar 9, 2024

0xkerem commented Mar 9, 2024

mkly commented Mar 9, 2024

0xkerem commented Mar 9, 2024

0xkerem commented Mar 11, 2024 • edited Loading

brianwgoldman Mar 11, 2024

Choose a reason for hiding this comment

brianwgoldman Mar 11, 2024

Choose a reason for hiding this comment

brianwgoldman Mar 11, 2024

Choose a reason for hiding this comment

0xkerem Mar 12, 2024

Choose a reason for hiding this comment

mkly commented Mar 11, 2024

brianwgoldman Mar 13, 2024

Choose a reason for hiding this comment

brianwgoldman Mar 13, 2024

Choose a reason for hiding this comment

0xkerem Mar 13, 2024

Choose a reason for hiding this comment

nathanw-mlc commented Mar 14, 2024 • edited Loading

0xkerem commented Mar 14, 2024

nathanw-mlc commented Mar 14, 2024

0xkerem commented Mar 14, 2024

0xkerem commented Mar 14, 2024

mkly commented Mar 19, 2024

0xkerem commented Mar 20, 2024

github-actions bot commented Mar 9, 2024 •

edited

Loading

0xkerem commented Mar 11, 2024 •

edited

Loading

nathanw-mlc commented Mar 14, 2024 •

edited

Loading