Skip to content

Conversation

@PaulHax
Copy link
Contributor

@PaulHax PaulHax commented Mar 25, 2025

Just leaving this here for now...

Mostly copy pastaed from #157 but made get_dialogs static so I don't have to load LLM to generate system prompt.

@PaulHax PaulHax force-pushed the outlines-system-prompt branch from 7322924 to ffbbcf5 Compare March 25, 2025 21:25
@PaulHax PaulHax changed the title Outlines system prompt Factor get_dialogs to static method in outlines_adm Mar 25, 2025
@PaulHax PaulHax force-pushed the outlines-system-prompt branch from ffbbcf5 to 45027fd Compare March 27, 2025 22:54
@PaulHax PaulHax force-pushed the outlines-system-prompt branch from 45027fd to e6ed50d Compare March 28, 2025 15:39
@PaulHax PaulHax requested a review from Copilot April 2, 2025 14:03
Copy link

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.

Pull Request Overview

This PR refactors parts of the outlines ADM by factoring out the get_dialogs functionality into a static method to avoid initializing the LLM unnecessarily. Key changes include:

  • Adding a reasoning_max_length parameter to action_choice_json_schema in outlines_prompts.py.
  • Converting _state_to_top_level_prompt and get_dialogs to static methods with updated signatures.
  • Adjusting baseline flag checks and related parameter propagation in top_level_choose_action.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
align_system/prompt_engineering/outlines_prompts.py Updated the JSON schema generator to allow dynamic setting of maxLength via reasoning_max_length.
align_system/algorithms/outlines_adm.py Refactored ADM prompt generation by moving key methods to static methods, and updated baseline handling and parameter propagation.
Comments suppressed due to low confidence (2)

align_system/algorithms/outlines_adm.py:247

  • [nitpick] The parameter 'baseline' is somewhat ambiguous. Consider renaming it (e.g., 'is_baseline') to clearly indicate that it is a flag.
def get_dialogs(scenario_state, available_actions, alignment_target, num_positive_samples=1, num_negative_samples=0, kdma_descriptions_map='align_system/prompt_engineering/kdma_descriptions.yml', shuffle_choices=True, baseline=False, scenario_description_template=scenario_state_description_1, action_selection_prompt_template=action_selection_prompt, baseline_system_prompt=baseline_system_prompt, **kwargs):

align_system/algorithms/outlines_adm.py:391

  • [nitpick] The instance method top_level_choose_action still references 'self.baseline', which is then passed to the static get_dialogs method. Ensure that the usage and naming of the baseline flag remains consistent across the code. Consider explicitly naming and commenting this behavior if needed.
if self.baseline and num_negative_samples > 0:

sampler=self.sampler,
whitespace_pattern=r"[ ]?")

if max_generator_tokens >= 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting approach, normally I would set the default in the kwargs to None and then if it's not None pass the value along as-is; but I guess in this case (setting the default to -1) you get to do some semantic validation on the value at the same time. Is that kind of the intention or is this just a tomato vs tomato.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The -1 is a gross hack, and I regret it already =)

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't say it's a hack, just curious about the tradeoffs; I'm content to leave it as is.

Comment on lines -282 to +296
positive_system_prompt = self.__class__.kdma_value_to_system_prompt(kdma, value)
negative_system_prompt = self.__class__.kdma_value_to_system_prompt(kdma, negative_value)
positive_system_prompt = OutlinesTransformersADM.kdma_value_to_system_prompt(kdma, value)
negative_system_prompt = OutlinesTransformersADM.kdma_value_to_system_prompt(kdma, negative_value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious why you prefer OutlinesTransformersADM over self.__class__ here (is the former just less obtuse but does the same thing? Or is there some case where one approach will break). Not asking for a change here just want to pick your brain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made that function static, so no self handy. Mabye there is some snakey black magic to get a __class__ in static func?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I missed that bit (not sure about getting __class__ in a static method) let's leave as is

Copy link
Contributor

@dmjoy dmjoy left a comment

Choose a reason for hiding this comment

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

Overall LGTM, happy to merge as is if you're ready.

@PaulHax PaulHax marked this pull request as ready for review April 2, 2025 19:00
@PaulHax
Copy link
Contributor Author

PaulHax commented Apr 2, 2025

OK, good to go by me, one less thing off my plate...

@dmjoy dmjoy merged commit 89f5835 into ITM-Kitware:main Apr 2, 2025
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.

2 participants