Skip to content

Conversation

@neubig
Copy link
Contributor

@neubig neubig commented May 13, 2025

This PR fixes issue #8493 by improving the environment variable casting logic for dict and list types.

The issue was in the utils.py file where the code was not properly handling direct dict or list types when casting environment variables. The fix adds support for both field_type is dict/list and get_origin(field_type) is dict/list cases, and uses json.loads first with a fallback to literal_eval for better compatibility.

Added a test case to verify the fix works correctly with dictionary environment variables.

Closes #8493

@neubig can click here to continue refining the PR


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:7fd735d-nikolaik   --name openhands-app-7fd735d   docker.all-hands.dev/all-hands-ai/openhands:7fd735d

+ ' "mem_limit": "2g",'
+ ' "cpu_count": 2,'
+ ' "environment": {"TEST_VAR": "test_value"}'
+ '}',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for fixing this! I'm not sure why does the agent assume this would be json, though?

The format is toml, these are attributes set in config.toml, and if the user wants, they can set and access them if set in env, instead.

@neubig neubig self-assigned this May 15, 2025
@neubig
Copy link
Contributor Author

neubig commented May 18, 2025

@OpenHands read @enysts comments and re-examine your assumptions about the data format. If you're assumptions are not correct please view the diff with the original code and revert anything to make the minimal changes possible to allow typing to pass

@openhands-ai
Copy link

openhands-ai bot commented May 18, 2025

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

@openhands-ai
Copy link

openhands-ai bot commented May 18, 2025

I've addressed the comment on PR #8494 regarding the environment variable casting issue. Here's a summary of what I did:

Changes Made

  1. Identified the Issue:

    • The PR had changed the parsing of dict and list types from environment variables to first try json.loads and then fall back to literal_eval.
    • @enyst questioned this approach, noting that the format is TOML for config files, not JSON.
  2. Analyzed the Codebase:

    • Examined how complex types were handled in existing tests and documentation.
    • Found that the SandboxConfig documentation mentioned JSON for docker_runtime_kwargs, but existing tests used Python literal syntax.
  3. Made Minimal Changes:

    • Reverted the parsing logic to use only literal_eval instead of trying json.loads first.
    • Updated the SandboxConfig documentation to specify "Python dictionary literal string" instead of "JSON string".
    • Kept the fix for supporting both field_type is dict/list and get_origin(field_type) is dict/list cases.
  4. Verified Changes:

    • Ran the test to ensure it still passes with the updated code.
    • Ran pre-commit hooks to ensure code quality.
  5. Committed and Pushed:

    • Committed the changes with a clear message.
    • Successfully pushed to the fix-env-var-casting-issue-8493 branch.

Conclusion

The changes successfully address @enyst's comment by:

  1. Removing the assumption that environment variables would be in JSON format
  2. Making minimal changes to the original code while still fixing the issue
  3. Ensuring type checking passes
  4. Maintaining consistency with the rest of the codebase

The PR now uses the same approach for parsing complex types as the rest of the codebase, while still fixing the original issue of supporting both direct and origin-based type checking for dicts and lists.

@neubig neubig requested a review from enyst May 19, 2025 18:09
@neubig
Copy link
Contributor Author

neubig commented May 19, 2025

Thanks @enyst fixed!

Copy link
Collaborator

@enyst enyst left a comment

Choose a reason for hiding this comment

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

Thank you!

@neubig neubig merged commit 6335afb into main May 20, 2025
18 checks passed
@neubig neubig deleted the fix-env-var-casting-issue-8493 branch May 20, 2025 21:47
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.

[Bug]: Casting environment variables in utils.py fails around line 99

4 participants