Conversation
…R to choose steps
dmjoy
left a comment
dmjoy
left a comment
There was a problem hiding this comment.
Overall this is in the right direction, but there are a few things I would like to see changed (see comments) or at least have a good handle on why the current modifications are needed for those cases.
| Unlike the Outlines engine, output is not grammar-constrained — the | ||
| schema is appended to the prompt as an instruction and the response | ||
| is parsed as JSON with a repair fallback. | ||
| """ |
There was a problem hiding this comment.
It does look like Ollama supports structured output with a JSON schema: https://ollama.com/blog/structured-outputs. Seems like we should use that if possible
| return outputs | ||
|
|
||
| def run_inference(self, prompts, schema): | ||
| def _parse_json(self, text: str) -> dict: |
There was a problem hiding this comment.
I'm not really excited about the idea of including this here. Have you been running into JSON validation etc. errors with the outlines inference engine here or? Just curious why this is even needed.
There was a problem hiding this comment.
I think it's fine to have the history tracking as you have it in here for now, but I'm more inclined to merge Yoni's approach on this: https://github.com/ITM-Kitware/align-system/pull/277/changes#diff-ea512e45fac46d4935ce85a4837bdbcd27b5891a09c1ac5f6aad038076f4d497
As it maintains the full working_output history.
| tool_lines = "\n".join(f"- {t.name}: {t.description}" for t in tools) | ||
| history_lines = ( | ||
| "\n".join(f"- {a.tool_name}({a.args})" for a in self._history) | ||
| if self._history else "None" | ||
| ) | ||
| predict_proposer_prompt = ( | ||
| f"Task: {scenario_state.unstructured}\n\n" | ||
| f"Available tools:\n{tool_lines}\n\n" | ||
| f"Action history:\n{history_lines}\n\n" | ||
| f"Generate {self.num_candidates} diverse candidate plans." | ||
| ) | ||
|
|
||
| score_schema = ( | ||
| '{"candidates":[{"actions":[{"tool_name":"MoveAhead","args":{"moveMagnitude":0.25}}],' | ||
| '"rationale":"..."}]}' | ||
| ) | ||
|
|
||
| prompt_system = ("You are an embodied planning model.\n" | ||
| "Return ONLY valid JSON. No extra text.\n" | ||
| f"Generate {self.num_candidates} semi-diverse candidate plans.\n") | ||
| prompt = ( | ||
| f"You are an embodied planning model.\n" | ||
| "Return ONLY valid JSON. No extra text.\n" | ||
| f"Generate {self.num_candidates} diverse candidate plans.\n" | ||
| f"- Each plan is 1 to {self.rollout_horizon} actions.\n" | ||
| f"- Use ONLY the tool names provided.\n" | ||
| f"- Args MUST satisfy each tool schema.\n" | ||
| f"- IMPORTANT objectId rule: For tools requiring objectId (TeleportNearObject, PickupObject, " | ||
| f"OpenObject, CloseObject, ToggleObjectOn/Off), you MUST copy the exact full objectId string " | ||
| "from the observation's visible lines (the value after 'id='). " | ||
| "Never use object type names like 'Apple' as objectId. Full objectIds contain '|' characters.\n" | ||
| "- Avoid repeating the same last action unless clearly helpful.\n" | ||
| ) |
There was a problem hiding this comment.
My preference would for these to be done similar to how we do prompts in other ADMs (outlines templates or callables, and parameterize them in the init call so that we can swap them around at Hydra configuration time)
There was a problem hiding this comment.
These experiment configs should be probably be in a subdirectory inside of experiment as that's typically how we've been doing it.
There was a problem hiding this comment.
I feel like these should be in a file specific to AI2Thor (if they are indeed specific data types for that domain) just to help with namespacing. I.e. if I do from align_system.data_models.ai2thor import ToolSpec that seems more informative
| Image.fromarray(frame.astype(np.uint8)).save(fpath) | ||
| print(f"[AI2ThorEnv] saved frame: {fpath}") | ||
|
|
||
| def reset(self, task: str) -> Observation: |
There was a problem hiding this comment.
All of the setup info etc. in here seems like it should be living in a data or config file somewhere rather than code?
| from align_system.data_models.types import Action as PlannerAction | ||
|
|
||
|
|
||
| TASKS = { |
There was a problem hiding this comment.
Similar story here, this should probably be in a data or configuration file somewhere and we would probably want to be able to modify it for a given experiment.
2 participants
…R to choose steps