Skip to content

Conversation

@neubig
Copy link
Contributor

@neubig neubig commented Apr 6, 2025

This PR adds more extensive typing to the openhands/core directory to improve type safety and code quality. The changes include:

  1. Adding return type annotations to functions that were missing them
  2. Adding parameter type annotations to function parameters
  3. Using more specific types instead of generic ones
  4. Adding type ignore comments for model_serializer decorators from pydantic
  5. Fixed typing in get_llm_config_from_agent to handle None values

These changes will help catch type-related bugs earlier and improve the developer experience with better IDE support.

Note: Only files in the openhands/core directory have been modified. All workflow and config files have been reverted to match the main branch.


To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:a82764a-nikolaik   --name openhands-app-a82764a   docker.all-hands.dev/all-hands-ai/openhands:a82764a

openhands-agent and others added 26 commits January 21, 2025 19:12
- Update mypy configuration with stricter type checking rules
- Add more type stubs to pre-commit configuration
- Run mypy both through pre-commit and directly in CI
- Install project in editable mode for better type checking
- Set correct PYTHONPATH in CI environment
@neubig neubig self-assigned this Apr 6, 2025
@neubig
Copy link
Contributor Author

neubig commented Apr 6, 2025

@OpenHands please check the type: ignore annotations that were added in this PR, and where it is possible to do so without extensive changes, remove these type ignore annotations and fix the underlying problem

@openhands-ai
Copy link

openhands-ai bot commented Apr 6, 2025

I'm on it! @neubig can track my progress at all-hands.dev

@neubig neubig marked this pull request as draft April 6, 2025 17:25
@openhands-ai
Copy link

openhands-ai bot commented Apr 6, 2025

I've successfully addressed the request to remove type: ignore annotations in the PR #7728 and fix the underlying typing issues. Here's a summary of the changes made:

Changes Made:

  1. Fixed typing in app_config.py:

    • Properly typed the agents dictionary as dict[str, AgentConfig] instead of just dict
    • Removed type: ignore annotations in get_agent_config() method by fixing the return type
    • Fixed get_llm_config_from_agent() method by adding a null check for llm_config_name to handle the case when it's None
  2. Fixed typing in message.py:

    • Replaced all # type: ignore annotations on @model_serializer decorators with proper type specifications
    • Added explicit mode='plain' parameter to all @model_serializer decorators to properly type the serializer functions

Verification:

  • Ran mypy type checking through the pre-commit hooks, which passed successfully for our changes
  • The pre-commit hooks did fail for other unrelated issues (ruff linting and formatting), but those were outside the scope of our task

Commit and Push:

  • Committed the changes with message "Remove type: ignore annotations and fix underlying typing issues"
  • Successfully pushed the changes to the fix/core-typing-improvements branch

All the type: ignore annotations that were identified in the PR comment have been successfully removed, and the underlying typing issues have been fixed without requiring extensive changes to the codebase.

@neubig neubig marked this pull request as ready for review April 6, 2025 19:49
@neubig neubig removed their assignment Apr 6, 2025
Comment on lines 126 to 127
def get_llm_config_from_agent(self, name: str | None = 'agent') -> LLMConfig:
agent_name = name if name is not None else 'agent'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if we remove the agent_name = ... line (and update references downstream to name) we can set the type of name directly to name: str = 'agent'.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't believe there's any inter-op issues between the typing imports like Dict or just using the class itself as an annotation (like dict), but the majority of the existing annotations use the latter style. Do we want to standardize on one or the other?

Copy link
Collaborator

@csmith49 csmith49 left a comment

Choose a reason for hiding this comment

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

A couple of notes but otherwise LGTM

@neubig
Copy link
Contributor Author

neubig commented Apr 8, 2025

@OpenHands please reflect csmith49's review comments, and also merge main and fix any merge conflicts

@openhands-ai
Copy link

openhands-ai bot commented Apr 8, 2025

I'm on it! @neubig can track my progress at all-hands.dev

@openhands-ai
Copy link

openhands-ai bot commented Apr 8, 2025

I've successfully addressed the PR comment by reflecting csmith49's review comments and merging main into the branch. Here's a summary of the changes made:

Changes Made:

  1. Addressed csmith49's review comments:

    • Fixed the get_llm_config_from_agent method in app_config.py by removing the unnecessary agent_name variable and simplifying the parameter type to just str instead of str | None
    • Standardized typing annotations in message.py by replacing imported types like Dict and List with built-in types like dict and list throughout the file
  2. Merged main into the branch and resolved conflicts:

    • Fixed merge conflicts in openhands/core/cli.py by adopting the newer implementation from main that uses prompt_toolkit instead of the older implementation
    • Added proper return type annotations to maintain type safety
  3. Fixed a type error in session.py:

    • Modified the _create_llm method to handle the None case before calling get_llm_config_from_agent, ensuring type compatibility
  4. Verified changes with pre-commit hooks:

    • Ran the pre-commit hooks to ensure our changes passed mypy type checking
    • Fixed all type errors related to our changes

All the requested changes have been successfully implemented and pushed to the fix/core-typing-improvements branch, updating PR #7728. The PR now reflects csmith49's review comments and has been merged with the latest changes from main.

@neubig neubig merged commit e698a39 into main Apr 8, 2025
16 checks passed
@neubig neubig deleted the fix/core-typing-improvements branch April 8, 2025 21:38
shabbir-shakudo pushed a commit to devsentient/OpenHands that referenced this pull request Jul 15, 2025
Co-authored-by: openhands <openhands@all-hands.dev>
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.

4 participants