OpenHands icon indicating copy to clipboard operation
OpenHands copied to clipboard

Make templates more customizable

Open enyst opened this issue 1 year ago • 2 comments

End-user friendly description of the problem this fixes or functionality that this introduces

  • [ ] Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below

Give a summary of what the PR does, explaining any non-trivial design decisions

This PR proposes to make the Jinja2 templates more customizable.

  • it breaks down examples.j2 from user_prompt.j2, to allow a different user prompt if wanted without affecting the examples and the other way around
  • it breaks down commands a bit more
  • agentskills refactoring of the way they're rendered
  • placeholders for tool use

Link of any specific issues this addresses

enyst avatar Oct 24 '24 09:10 enyst

@xingyaoww I'm thinking that this is just a step. We need some more splitting IMHO - I restructured a bit more, in blocks too, but I didn't get to finish/test those.

In addition to these user and default directories, how do you think about one .md/.j2 file per block (e.g. load EXECUTE_BASH from user-directory if present, otherwise default, etc for all blocks we can figure out), would it be too granular?

enyst avatar Oct 24 '24 12:10 enyst

Overall Feedback:

The PR introduces significant improvements to the customization of Jinja2 templates, enhancing flexibility and modularity. The changes are well-structured, allowing for more granular control over template components. However, there are areas where error handling and code clarity can be improved, particularly in the new functions and template rendering logic.

Score: 85/100

Labels: Enhancement, Tests

Code Suggestions:

  1. File: openhands/agenthub/codeact_agent/agent_skills.j2

    • Language: Jinja
    • Suggestion Content: Add error handling for missing skill documentation.
    • Relevant Line: +{{ get_skill_docstring(skill_name) }}
    • Existing Code:
      {{ get_skill_docstring(skill_name) }}
      
    • Improved Code:
      {% set docstring = get_skill_docstring(skill_name) %}
      {{ docstring if docstring else 'Documentation not available' }}
      
  2. File: openhands/agenthub/codeact_agent/codeact_agent.py

    • Language: Python
    • Suggestion Content: Improve error handling in _get_skill_docstring to log errors instead of printing them.
    • Relevant Line: + print(e)
    • Existing Code:
      print(e)
      
    • Improved Code:
      logging.error(f"Error retrieving skill docstring for {skill_name}: {e}")
      
  3. File: tests/unit/test_prompt_manager.py

    • Language: Python
    • Suggestion Content: Ensure that the test for loading agent skills checks for all expected skills.
    • Relevant Line: + assert ('open_file(path: str, line_number: int | None = 1, context_lines: int | None = 100) -> None' in manager.system_message)
    • Existing Code:
      assert ('open_file(path: str, line_number: int | None = 1, context_lines: int | None = 100) -> None' in manager.system_message)
      
    • Improved Code:
      expected_skills = [
          'open_file(path: str, line_number: int | None = 1, context_lines: int | None = 100) -> None',
          # Add other expected skills here
      ]
      for skill in expected_skills:
          assert skill in manager.system_message
      

The PR enhances the customization of Jinja2 templates effectively. Here are some suggestions:

  1. In agent_skills.j2, consider adding error handling for missing skill documentation.
  2. Improve error handling in _get_skill_docstring by logging errors instead of printing them.
  3. Ensure that the test for loading agent skills checks for all expected skills.

ngduyanhece avatar Oct 24 '24 14:10 ngduyanhece