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

feat: added response conversion node #318

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

Conversation

tanweersalah
Copy link
Contributor

@tanweersalah tanweersalah commented Dec 23, 2024

Description

Changes proposed in this pull request:

  • Added response conversion functionality to finalizer

Related issue(s)

#276

Copy link

Note(s) for PR Auther:

  • The integration test will be skipped for the PR. You can trigger it manually after adding the label: run-integration-test.
  • The evaluation test will be skipped for the PR. You can trigger it manually after adding the label: evaluation requested.
  • If any changes are made to the evaluation tests data, make sure that the integration tests are working as expected.
  • If any changes are made to how to run the unit tests, make sure to update the steps for unit-tests in the create-release.yml workflow as well.

Note(s) for PR Reviewer(s):

  • Make sure that the integration and evaluation tests are working as expected.

@kyma-bot kyma-bot added size/XXL and removed size/XL labels Dec 27, 2024
@tanweersalah tanweersalah marked this pull request as ready for review December 27, 2024 10:07
@tanweersalah tanweersalah requested a review from a team as a code owner December 27, 2024 10:07
@tanweersalah tanweersalah requested a review from muralov December 27, 2024 10:07
@tanweersalah tanweersalah linked an issue Dec 27, 2024 that may be closed by this pull request
2 tasks
Comment on lines +55 to +85
def _parse_yamls(self, yaml_config: str) -> Any | None:
"""
Parse YAML string into Python object with error handling.
Attempts two parsing methods:
1. First check: if yaml markers available
- tries parsing after removing yaml markers
2. Else, tries parsing the raw string

Args:
yaml_config: YAML configuration string

Returns:
Parsed YAML object or None if parsing fails
"""
try:
# First check: if yaml markers available
if yaml_config[:7] == "```yaml":
# parsing after removing yaml markers
parsed_yaml = yaml.safe_load(yaml_config[8:-4])
else:
# Parse raw string
parsed_yaml = yaml.safe_load(yaml_config)

except Exception as e:
logger.error(
f"Error while parsing the yaml : {yaml_config} , Exception : {e}"
)

return None

return parsed_yaml
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's use TypedDict or Pydantic to parse:

class YAMLMetadata(TypedDict):
    namespace: str
    name: str

class YAMLConfig(TypedDict):
    metadata: YAMLMetadata
    kind: str

def _parse_yamls(self, yaml_config: str) -> Optional[YAMLConfig]:
    """
    Parse YAML string into Python object with error handling.
    Returns None if parsing fails.

    Args:
        yaml_config: YAML configuration string

    Returns:
        Parsed YAML object or None if parsing fails
    
    Example:
        >>> result = converter._parse_yamls("```yaml\\nkind: Deployment\\n```")
        >>> if result:
        >>>     print(f"Parsed YAML: {result}")
        >>> else:
        >>>     print("Failed to parse YAML")
    """
    try:
        if yaml_config[:7] == "```yaml":
            parsed_yaml = yaml.safe_load(yaml_config[8:-4])
        else:
            parsed_yaml = yaml.safe_load(yaml_config)
        
        return parsed_yaml

    except Exception as e:
        logger.error(
            f"Error while parsing the yaml : {yaml_config} , Exception : {e}"
        )
        return None

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I see if a yaml is generated but doesn't fit the schema, it returns None.
Have you observed the cases where partial yaml resources are generated? We can discuss this.

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 agreed will skip this

Copy link
Collaborator

Choose a reason for hiding this comment

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

are both _generate_final_response and _convert_final_response methods are shown in Langfuse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@muralov muralov left a comment

Choose a reason for hiding this comment

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

Please rebase on main, there is conflict.

I'd add such instructions for prompts so that LLM outputs fit your logic. We can discuss this.

  1. For new resource deployments, wrap the YAML in <YAML-NEW> </YAML-NEW> tags
  2. For modifications to existing resources, wrap the YAML in <YAML-UPDATE> </YAML-UPDATE> tags
  3. Each YAML configuration must include:
     - apiVersion
     - kind
     - metadata (with at least name and namespace)
  4. Multiple YAML configurations should be separated into individual blocks with appropriate tags
  
    Example:
  <YAML-NEW>
  apiVersion: apps/v1
  kind: Deployment
  metadata:
    name: my-new-app
    namespace: default
  spec:
    ...
  </YAML-NEW>

  <YAML-UPDATE>
  apiVersion: v1
  kind: Service
  metadata:
    name: existing-service
    namespace: production
  spec:
    ...
  </YAML-UPDATE>

@kyma-bot kyma-bot added size/XL and removed size/XXL labels Jan 23, 2025
@muralov muralov added the evaluation requested A PR with this label will trigger the validation workflow. label Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes evaluation requested A PR with this label will trigger the validation workflow. run-integration-test size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Develop response conversion node
3 participants