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

AAP-36732: ModelPipelines: Configuration improvements: Remove env vars #1487

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

manstis
Copy link
Contributor

@manstis manstis commented Jan 10, 2025

Jira Issue: https://issues.redhat.com/browse/AAP-36732

Description

This PR removes the discrete (legacy) environment variables previously used to configure LLM services (WCA, Ollama, GRPC etc). All configuration is now controlled by a JSON (or YAML) block, stored in a ANSIBLE_AI_MODEL_MESH_CONFIG environment. Various examples are given in this PR.

I have included support for the legacy use of environment variables.

These are used if ANSIBLE_AI_MODEL_MESH_CONFIG is not set.

This is predominantly for the use by the Operator; where an older version uses the latest service version.

Testing

N/A. Internal refactoring of existing code.

Steps to test

N/A. Internal refactoring of existing code.

Scenarios tested

N/A. Internal refactoring of existing code.

Production deployment

  • This code change is ready for production on its own
  • This code change requires the following considerations before going to production:

@manstis manstis force-pushed the AAP-36732 branch 2 times, most recently from 5a88ec7 to 3051acc Compare January 10, 2025 12:53
@manstis manstis changed the title [WIP] AAP-36732: ModelPipelines: Configuration improvements: Remove env vars [DRAFT] AAP-36732: ModelPipelines: Configuration improvements: Remove env vars Jan 10, 2025
@manstis manstis requested review from goneri and jameswnl January 10, 2025 12:57
@manstis manstis changed the title [DRAFT] AAP-36732: ModelPipelines: Configuration improvements: Remove env vars AAP-36732: ModelPipelines: Configuration improvements: Remove env vars Jan 13, 2025
@manstis manstis marked this pull request as ready for review January 13, 2025 13:56
@manstis manstis force-pushed the AAP-36732 branch 3 times, most recently from 0d0074e to 072a815 Compare January 13, 2025 19:36
@manstis manstis force-pushed the AAP-36732 branch 2 times, most recently from 17cba50 to de5afa1 Compare January 14, 2025 13:10
@goneri
Copy link
Contributor

goneri commented Jan 15, 2025

Work as expected with the following configuration:

export ANSIBLE_AI_MODEL_MESH_CONFIG="
---
ModelPipelineCompletions:
  provider: dummy
ModelPipelineContentMatch:
  provider: dummy
ModelPipelinePlaybookGeneration:
  provider: dummy
ModelPipelineRoleGeneration:
  provider: dummy
ModelPipelinePlaybookExplanation:
  provider: dummy
ModelPipelineChatBot:
  provider: dummy
"

The part I found a bit annoying is that if my YAML syntax is incorrect, the exception is silently ignored.

goneri
goneri previously approved these changes Jan 15, 2025
@manstis
Copy link
Contributor Author

manstis commented Jan 15, 2025

@goneri

The part is dislike I found a bit annoying is that if my YAML syntax is incorrect, the exception is silently ignored.

That'll be caused by this where exceptions are effectively ignored.

Let me tinker a little before this is merged.

@goneri
Copy link
Contributor

goneri commented Jan 15, 2025

@goneri

The part is dislike I found a bit annoying is that if my YAML syntax is incorrect, the exception is silently ignored.

That'll be caused by this where exceptions are effectively ignored.

Let me tinker a little before this is merged.

Actually my configuration doesn't work because the config keys are missing:

[django]     |   File "/var/www/ansible-ai-connect-service/ansible_ai_connect/ai/api/model_pipelines/config_serializers.py", line 38, in to_internal_value
[django]     |     serializer = REGISTRY[provider_part["provider"]][Serializer](data=data["config"])
[django]     |                                                                       ~~~~^^^^^^^^^^
[django]     | KeyError: 'config'

@manstis
Copy link
Contributor Author

manstis commented Jan 15, 2025

@goneri

Actually my configuration doesn't work because the config keys are missing.

Excellent find. Thanks.

@manstis
Copy link
Contributor Author

manstis commented Jan 16, 2025

@goneri PR updated to better handle exceptions (and fixed KeyError: 'config').

@manstis manstis force-pushed the AAP-36732 branch 2 times, most recently from 8c0c083 to 154c04c Compare January 16, 2025 08:59
@@ -30,7 +30,7 @@ jobs:
- name: Set up python3
uses: actions/setup-python@v4
with:
python-version: '3.10'
python-version: '3.11'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This aligns the Python version with the other GHA definition.

3.11 is needed to support ExceptionGroup (used in this PR). It is also the version used to build the container.

@manstis manstis added the pr-deployment To trigger a deployment based on this PR's image label Jan 16, 2025
@jameswnl
Copy link
Contributor

PR Deployment created in namespace wisdom-pr-1487

Look up the wisdom-service url in the cluster

@manstis manstis added pr-deployment To trigger a deployment based on this PR's image and removed pr-deployment To trigger a deployment based on this PR's image labels Jan 16, 2025
@manstis
Copy link
Contributor Author

manstis commented Jan 16, 2025

See https://issues.redhat.com/browse/AAP-38734 re: failing "PR Deployment Testing" check.

I have however deployed the container built by this PR to stage2-west manually:

  • namespace: manstis
  • instance: my-aiconnect
  • I won't paste routes etc on Git Hub.

@manstis manstis requested a review from goneri January 17, 2025 11:35
@manstis manstis merged commit 9e01eb9 into main Jan 17, 2025
15 of 17 checks passed
@manstis manstis deleted the AAP-36732 branch January 17, 2025 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-deployment To trigger a deployment based on this PR's image
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants