-
Notifications
You must be signed in to change notification settings - Fork 113
[ENHANCEMENT] migrate to pydantic models and add parameterized reset/step support #217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release
Are you sure you want to change the base?
Conversation
|
@burtenshaw could you review this when you get the opportunity, thanks! |
|
Nice work again @rycerzes ! I've tested this PR on the echo env with That said, this does break all server <> client compatibility so I would like to release this as
I'll discuss with @zkwentz how to set this up. But for now, I would work on websockets from this branch and we'll sort it out from there. |
|
Hi @rycerzes ! Can you offer more background as of why you want to move to Pydantic? We generally want to stick to clean Python as much as possible, and config logic can always happen at the config layer with a tool like Hydra. In general, I would say that opening a RFC may be the better option for these deep changes to core so that we can discuss before you write code (though maybe in this day and age you can have an LLM write code so maybe that's ok :) ). As things are for now, I would reject this PR but I want to understand some more of the background to see what we can do |
Darktex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocked until we discuss some more
|
Thanks for the context, @Darktex! I appreciate you taking the time to review this. You raise valid concerns about introducing Pydantic as a dependency. The primary motivation comes from issue #182, where @burtenshaw identified that the current dataclass approach leads to poor error handling (HTTP 500s instead of proper validation errors) and missing auto-generated documentation for the web interface. The core problems I was trying to solve are validation failures that result in cryptic errors, lack of proper OpenAPI documentation, and the need for manual form generation in the web interface. Pydantic addresses these directly since our HTTP server already uses FastAPI, which is built on Pydantic. That said, I completely understand the preference for minimal dependencies. You're absolutely right that an RFC would have been the better approach for such a foundational change. I jumped straight into implementation when I should have discussed the architectural direction first. Would you prefer I close this PR and open an RFC to discuss alternatives? We could explore lighter-weight solutions or find a way to address the validation and documentation issues without the full Pydantic migration. The parameterized reset/step functionality could potentially be implemented separately in a different PR with simpler validation. I'm happy to work within whatever architectural constraints the team prefers. |
|
@Darktex I would also add that pydantic is the standard for FastAPI and standard practice in modern python web APIs. |
Yes and the
|
This PR addresses issues #182 and #183 by migrating all data models from dataclasses to Pydantic BaseModel and adding support for parameterized environment reset and step operations. This provides enhanced validation, better error handling, auto-generated documentation, and flexible environment control.
Changes Made
Core Infrastructure Migration (#182)
types.py: Migrated all base classes (Action, Observation, State, CodeExecResult, EnvironmentMetadata) from dataclasses to Pydantic modelsConfigDictResetRequest,ResetResponse,StepRequest,StepResponse)http_server.py: Enhanced HTTP server with comprehensive API documentation and validation/schema/action,/schema/observation,/schema/state)/metadata) for environment informationweb_interface.py: Updated web interface to use Pydantic serializationmodel_dump()instead ofasdict()Parameterized Environment Operations (#183)
interfaces.py: Updated Environment interface to support flexible parametersreset()method now accepts optionalseed,episode_id, and custom parametersstep()method now accepts optionaltimeout_sand custom parametersget_metadata()method for environment informationhttp_env_client.py: Enhanced client to forward arbitrary parametersreset()method forwards all keyword arguments to serverstep()method forwards all keyword arguments to serverEnvironment Updates
Echo Environment: Updated to use new Pydantic models
EchoActionandEchoObservationto Pydantic with field validationAPI Enhancements
New Endpoints
GET /schema/action- JSON schema for action validationGET /schema/observation- JSON schema for observation structureGET /schema/state- JSON schema for state representationGET /metadata- Environment metadata and informationEnhanced Existing Endpoints
POST /reset- Now accepts optional parameters (seed, episode_id, etc.)POST /step- Now accepts optional parameters (timeout_s, request_id, etc.)Closes #182
Closes #183
TODOs:
echo_envto be compatible with the new changes and serve as reference for updating the rest