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

Python: Adgudime/python/serialization/settings #1312

Conversation

AdityaGudimella
Copy link
Contributor

@AdityaGudimella AdityaGudimella commented Jun 2, 2023

Motivation and Context

This PR is the first of the PRs to use Pydantic in SemanticKernel. Any class that is a subclass
of pydantic.BaseModel is json serializable. This will be used to serialize the Kernel class.
Serialization of the Kernel is required for MLFlow integration.

Description

New dependencies:

  • python-dotenv: Required for pydantic to support dotenv files.

Breaking Changes:

  • .env file config keys have changed. See .env.example file for new keys
  • Env Var OpenAI__ApiKey is no longer used in tests.
    It needs to be replaced in GitHub secrets with OPENAI_API_KEY
  • Removed semantic_kernel.utils.settings::openai_settings_from_dot_env
  • Removed get_oai_config fixture from tests/conftest.py

Other changes:

  • (OpenAI) Settings can be loaded from
    • yaml files,
    • env vars, and,
    • .env files
      (in the same order of precedence)
  • Added tests for
    -new functionality, and
    • to ensure (some of) the old functionality hasn't changed.

Next steps:
Repeat this PR for AzureOpenAISettings

Contribution Checklist

New dependencies:
- python-dotenv: Required for pydantic to support dotenv files.

Breaking Changes:
- .env file config keys have changed. See .env.example file for new keys
- Env Var `OpenAI__ApiKey` is no longer used in tests.
  It needs to be replaced in GitHub secrets with `OPENAI_API_KEY`
- Removed `semantic_kernel.utils.settings::openai_settings_from_dot_env`
- Removed `get_oai_config` fixture from `tests/conftest.py`

Other changes:
- (OpenAI) Settings can be loaded from
  - yaml files,
  - env vars, and,
  - .env files
  (in the same order of precedence)
- Added tests for
   -new functionality, and
   - to ensure (some of) the old functionality hasn't changed.

Next steps:
Repeat this PR for `AzureOpenAISettings`
@github-actions github-actions bot added the python Pull requests for the Python Semantic Kernel label Jun 2, 2023
def create_kernel():
kernel = sk.Kernel()
return kernel


@pytest.fixture(scope="session")
@pytest.fixture()
Copy link
Contributor

Choose a reason for hiding this comment

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

rationale for removing scope="session" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yet another thing I forgot to mention. Apologies. I wanted to ask you about this, and made this change so that I don't forget it.

Did you intend to use scope="session" in the fixture? The default scope is "function" which basically means that the fixture gets recreated for every test. Other fixture scopes, in my experience, cause issues down the line, once the number of tests increases. Since non-function scope fixtures are shared across multiple tests, if any one of those tests makes a change to the result returned by the fixture, downstream tests see the changed value. And since the order in which the tests are run is not fixed (especially across code changes), it sometimes leads to failures which are not easily reproducible either.

Fixtures with larger scopes are typically used when the object being constructed in the fixture is heavy and takes time to be created or if you need the shared property of the fixture. These fixtures don't seem to be satisfying either of these criteria, so I just wanted to check with you on them.

@awharrison-28 awharrison-28 changed the base branch from main to feature-pydantic June 9, 2023 21:21
@awharrison-28 awharrison-28 self-assigned this Jun 9, 2023
@awharrison-28
Copy link
Contributor

Updated the target branch to feature-pydantic

@github-actions github-actions bot added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel kernel.core labels Jun 14, 2023
@github-actions github-actions bot removed kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code samples labels Jun 14, 2023
@shawncal shawncal changed the title Adgudime/python/serialization/settings Python: Adgudime/python/serialization/settings Jun 29, 2023
@shawncal
Copy link
Member

shawncal commented Jul 8, 2023

@awharrison-28 I see some failing UTs. Shall we help out @AdityaGudimella by merging to the feature-pydantic branch, and fixing the last few problems there?

@awharrison-28
Copy link
Contributor

Moving this to draft

@awharrison-28 awharrison-28 marked this pull request as draft July 19, 2023 23:31
@shawncal
Copy link
Member

shawncal commented Aug 3, 2023

@AdityaGudimella @awharrison-28 Closing as I understand this PR is superseded by the new ones. Please confirm or reopen if this PR is still applicable.

@shawncal shawncal closed this Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Pull requests for the Python Semantic Kernel
Projects
Archived in project
Status: Sprint: Done
Development

Successfully merging this pull request may close these issues.

5 participants