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

Simulator using structured outputs for conversation and conversation ID #39337

Conversation

nagkumar91
Copy link
Member

@nagkumar91 nagkumar91 commented Jan 21, 2025

Description

Please add an informative description that covers that changes made by the pull request and link all relevant issues.

If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@nagkumar91 nagkumar91 requested a review from a team as a code owner January 21, 2025 22:40
@github-actions github-actions bot added the Evaluation Issues related to the client library for Azure AI Evaluation label Jan 21, 2025
@nagkumar91 nagkumar91 requested a review from Copilot January 21, 2025 22:43
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • sdk/evaluation/azure-ai-evaluation/azure/ai/evaluation/simulator/_adversarial_simulator.py: Evaluated as low risk
Comments suppressed due to low confidence (5)

sdk/evaluation/azure-ai-evaluation/azure/ai/evaluation/simulator/_conversation/init.py:129

  • Remove commented-out print statements to keep the code clean.
print(self.persona_template_args)

sdk/evaluation/azure-ai-evaluation/azure/ai/evaluation/simulator/_conversation/init.py:135

  • Remove commented-out print statements to keep the code clean.
print(f"Conversation starter content: {conversation_starter_content}")

sdk/evaluation/azure-ai-evaluation/azure/ai/evaluation/simulator/_conversation/init.py:140

  • Remove commented-out print statements to keep the code clean.
print("Successfully created a Jinja2 template for the conversation starter.")

sdk/evaluation/azure-ai-evaluation/azure/ai/evaluation/simulator/_conversation/init.py:141

  • Remove commented-out print statements to keep the code clean.
print(f"Template syntax error: {e}. Using raw content.")

sdk/evaluation/azure-ai-evaluation/azure/ai/evaluation/simulator/_conversation/_conversation.py:177

  • [nitpick] The logger warning message could be more descriptive.
logger.warning("Error: %s", str(e))

turn_number=0,
conversation_id=conversation_id,
)
except Exception: # pylint: disable=broad-except
Copy link
Preview

Copilot AI Jan 21, 2025

Choose a reason for hiding this comment

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

Avoid using broad exception handling. Handle specific exceptions instead.

Suggested change
except Exception: # pylint: disable=broad-except
except SomeSpecificException: # Replace SomeSpecificException with the actual exception type

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

max_history=history_limit,
turn_number=current_turn,
)
except Exception: # pylint: disable=broad-except
Copy link
Preview

Copilot AI Jan 21, 2025

Choose a reason for hiding this comment

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

Avoid using broad exception handling. Handle specific exceptions instead.

Suggested change
except Exception: # pylint: disable=broad-except
except (TimeoutError, ValueError): # pylint: disable=broad-except

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@nagkumar91 nagkumar91 requested a review from Copilot January 22, 2025 16:05
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 8 changed files in this pull request and generated 1 comment.

Files not reviewed (3)
  • sdk/evaluation/azure-ai-evaluation/tests/unittests/test_synthetic_conversation_bot.py: Evaluated as low risk
  • sdk/evaluation/azure-ai-evaluation/CHANGELOG.md: Evaluated as low risk
  • sdk/evaluation/azure-ai-evaluation/azure/ai/evaluation/simulator/_conversation/init.py: Evaluated as low risk
Comments suppressed due to low confidence (1)

sdk/evaluation/azure-ai-evaluation/azure/ai/evaluation/simulator/_model_tools/models.py:618

  • Returning a hardcoded "id": "unknown" could lead to confusion when debugging or tracking conversations. Consider logging a warning or raising an exception if the id is not found in the response data.
return {"samples": [response_data], "finish_reason": ["completed"], "id": "unknown"}

@@ -350,6 +357,7 @@ def _setup_bot(
parameters: TemplateParameters,
target: Optional[Callable] = None,
scenario: Union[AdversarialScenario, AdversarialScenarioJailbreak],
conversation_id: Optional[str] = None,
Copy link
Preview

Copilot AI Jan 22, 2025

Choose a reason for hiding this comment

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

The parameter 'conversation_id' is added but not used in the '_setup_bot' method. It should be either removed or utilized properly.

Suggested change
conversation_id: Optional[str] = None,
scenario: Union[AdversarialScenario, AdversarialScenarioJailbreak],

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

@nagkumar91 nagkumar91 closed this Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Evaluation Issues related to the client library for Azure AI Evaluation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants