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

Update config_list_from_json function to support YAML parsing #2560

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Mai0313
Copy link
Collaborator

@Mai0313 Mai0313 commented May 1, 2024

Why are these changes needed?

Add Yaml file supported for OAI_CONFIG_LIST by simply changing from json.load to yaml.safe_load

Related issue number

#2559

Checks

@Mai0313
Copy link
Collaborator Author

Mai0313 commented May 1, 2024

@microsoft-github-policy-service agree

@microsoft-github-policy-service agree company="Mediatek"

@Mai0313
Copy link
Collaborator Author

Mai0313 commented May 2, 2024

Idk why there is no yaml module, i thought it was a default module for Python

Copy link
Collaborator

@ekzhu ekzhu 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! Could you also update the documentation: website/docs/topics/llm_configuration.ipynb

@codecov-commenter
Copy link

codecov-commenter commented May 6, 2024

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 15.75%. Comparing base (372ac1e) to head (b97b99d).
Report is 3 commits behind head on main.

Files Patch % Lines
autogen/oai/openai_utils.py 20.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2560       +/-   ##
===========================================
- Coverage   33.11%   15.75%   -17.37%     
===========================================
  Files          86       86               
  Lines        9108     9109        +1     
  Branches     1938     2090      +152     
===========================================
- Hits         3016     1435     -1581     
- Misses       5837     7626     +1789     
+ Partials      255       48      -207     
Flag Coverage Δ
unittests 15.75% <20.00%> (-17.37%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Mai0313
Copy link
Collaborator Author

Mai0313 commented May 6, 2024

website/docs/topics/llm_configuration.ipyn

oh sorry, it is my first time to use mdx format, is there anyway I can preview the changes before commit?
I tried this extension, but it seems not working for me.

- Add pytest for load_config_from_json for yaml format
- Add docs for website
- Add pyyaml to setup.py
Copy link

gitguardian bot commented May 6, 2024

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
10404695 Triggered Generic High Entropy Secret b97b99d test/oai/test_utils.py View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@Mai0313
Copy link
Collaborator Author

Mai0313 commented May 6, 2024

@ekzhu
It is my first time to use mdx format and I do not know where I can preview the changes for mdx, so basically, the way I have done is copy-paste the previous content and changed to yaml version.

BTW, I have added pyyaml to setup.py to make sure no one get error when loading config, since we don't use json module but yaml.

Additionally, I have implemented pytest for the YAML version. Specifically, I utilized pytest.mark.parametrize to minimize code duplication.

@Mai0313 Mai0313 requested a review from ekzhu May 11, 2024 08:46
@ekzhu
Copy link
Collaborator

ekzhu commented May 13, 2024

It is my first time to use mdx format and I do not know where I can preview the changes for mdx, so basically, the way I have done is copy-paste the previous content and changed to yaml version.

See https://microsoft.github.io/autogen/docs/contributor-guide/documentation, the process_notebooks.py script will generate the MDX files.

@@ -25,6 +25,7 @@
# Disallowing 2.6.0 can be removed when this is fixed https://github.com/pydantic/pydantic/issues/8705
"pydantic>=1.10,<3,!=2.6.0", # could be both V1 and V2
"docker",
"pyyaml",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can add pyyaml to test dependencies. But for install dependency let's move this to an optional dependency like those below.

else:
# Else, it should be a JSON string by itself.
json_str = env_str
config_list = json.loads(json_str)
json_or_yaml_str = env_str
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the yaml dependency is not installed, we can fall back to use json module. We can do this by checking for yaml import error during import, and use that information to choose whether to use yaml load or json load.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh, it sounds good

@@ -475,21 +476,38 @@ def config_list_from_json(

Args:
env_or_file (str): The name of the environment variable, the filename, or the environment variable of the filename
that containing the JSON data.
that containing the JSON/YAML data.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add explanation for installing pyyaml.

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.

None yet

3 participants