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

MAINT: Refactor Skeleton Key to be a subclass of PromptSendingOrchest… #650

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Maalvi14
Copy link

Overview

This PR refactors the SkeletonKeyOrchestrator to properly inherit from PromptSendingOrchestrator instead of Orchestrator. The refactoring improves code organization and reliability while maintaining all existing functionality. It focuses on leveraging the parent class's capabilities for prompt handling and conversation management.

Key Features

Method Refactoring:

  • Rewrote send_skeleton_key_with_prompts_async to use parent class's batching capabilities
  • Enhanced send_skeleton_key_with_prompt_async to maintain consistent conversation context
  • Removed redundant prompt sending logic by leveraging parent class methods
  • Introduced proper use of _create_normalizer_request for request creation

Request Handling:

  • Added single conversation ID management across all prompts in a sequence
  • Implemented proper metadata propagation through request chain
  • Enhanced RPM validation to use correct attribute checks
  • Improved batch processing of skeleton key and attack prompts

Memory Management:

  • Leveraged parent class's memory tracking functionality
  • Fixed duplicate memory entries in conversation history
  • Ensured proper conversation linking between skeleton key and attack prompts
  • Maintained correct memory entry count for all scenarios

Backward Compatibility:

  • Maintained existing public API interface
  • Preserved custom skeleton key functionality
  • Retained support for prompt converters
  • Kept existing RPM validation capabilities

Related Issues

MAINT: Refactor Skeleton Key to be a subclass of PromptSendingOrchestrator #599

Next Steps (DONE):

  • ✅ Refactored core methods to use PromptSendingOrchestrator functionality
  • ✅ Fixed conversation ID handling in both single and multi-prompt scenarios
  • ✅ Corrected memory entry management
  • ✅ Updated RPM validation logic
  • ✅ Verified all 13 unit tests pass successfully
  • ✅ Confirmed backward compatibility with existing features

@Maalvi14
Copy link
Author

@microsoft-github-policy-service agree

@@ -23,11 +23,12 @@
PromptConverterConfiguration,
)
from pyrit.prompt_target import PromptTarget
from pyrit.orchestrator.single_turn.prompt_sending_orchestrator import PromptSendingOrchestrator
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from pyrit.orchestrator.single_turn.prompt_sending_orchestrator import PromptSendingOrchestrator
from pyrit.orchestrator.single_turn import PromptSendingOrchestrator


logger = logging.getLogger(__name__)


class SkeletonKeyOrchestrator(Orchestrator):
class SkeletonKeyOrchestrator(PromptSendingOrchestrator):
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend moving this file to pyrit/orchestrator/single_turn

Copy link
Contributor

Choose a reason for hiding this comment

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

(and as part of that move, updating init.py

"""
if hasattr(self._prompt_target, 'rpm') and self._prompt_target.rpm and self._batch_size != 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be removed - it's the responsibility of the target to check this

conversation_id = str(uuid4())
metadata = {"conversation_id": conversation_id}
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the conversation_id as metadata. PromptRequestPieces have this as an attribute and it can be set there


# Return the attack prompt response (second response)
return responses[1]

def print_conversation(self) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

you should be able to remove print_conversation and use the parent class


Returns:
list[PromptRequestResponse]: The responses from the prompt target.
PromptRequestResponse: The response from the prompt target.
"""
Copy link
Contributor

@rlundeen2 rlundeen2 Jan 20, 2025

Choose a reason for hiding this comment

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

Instead of doing it like how it was done, I would make use of PromptSendingOrchestrator.

  1. Use the set_prepended_conversation to set the skeleton key. This can be done in init
  2. You can remove the send_skeleton_key_with_prompt_async function. Sending any prompt will then have the skeleton key prepended

prompt_converters=prompt_converters,
batch_size=batch_size,
verbose=verbose
)

Copy link
Contributor

Choose a reason for hiding this comment

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

pre-commit is failing. You can run it using pre-commit run --all-files; And you can see the current issues in the CI build: https://github.com/Azure/PyRIT/actions/runs/12876675701/job/35900136914?pr=650

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.

MAINT: Refactor Skeleton Key to be a subclass of PromptSendingOrchestrator
2 participants