fix(api): use config_to_dict() for ROMAConfig serialization in ExecutionService#95
Open
0xghost42 wants to merge 1 commit into
Open
fix(api): use config_to_dict() for ROMAConfig serialization in ExecutionService#950xghost42 wants to merge 1 commit into
0xghost42 wants to merge 1 commit into
Conversation
…ionService Closes sentient-agi#71. Reported in sentient-agi#71: POST /api/v1/executions returns 500 with {"detail":"Failed to create execution: 'ROMAConfig' object is not iterable"} regardless of profile. The root cause was diagnosed in sentient-agi#74 by @nguyenhuulocbb: ExecutionService.start_execution falls through a `hasattr(config, 'model_dump')` guard to `dict(config)` when persisting the execution record. ROMAConfig is OmegaConf-structured (not a Pydantic model with model_dump and not iterable as a mapping), so the `dict(config)` branch raises TypeError before storage.create_execution is ever called. Use config_to_dict() from roma_dspy.config.utils, which is the helper the rest of the codebase already uses for the same conversion (see get_config_diff in the same module). It handles both ROMAConfig and plain DictConfig inputs via OmegaConf.to_container. Tests - Updated the mock_config_manager fixture in test_execution_service to return a real OmegaConf DictConfig instead of a MagicMock with a pre-canned model_dump. This exercises the real serialisation path and would have caught the regression at its introduction.
Author
|
Hi @salzubi401 — gentle bump if this repo is still on your radar. PR has been open since 13/05; fixes ExecutionService serializing a Pydantic ROMAConfig with the wrong helper (uses to_dict()/dict instead of config_to_dict(), so nested models leak through). Small, scoped change. Happy to rebase or split if needed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #71.
POST /api/v1/executionsreturns a 500 regardless of profile:{"detail":"Failed to create execution: 'ROMAConfig' object is not iterable"}The root cause was diagnosed in #74 by @nguyenhuulocbb (credit to them — no PR had been opened):
ExecutionService.start_executionfalls through ahasattr(config, "model_dump")guard todict(config)when persisting the execution record.ROMAConfigis OmegaConf-structured — it is not a Pydantic model withmodel_dumpand is not iterable as a mapping — so thedict(config)branch raisesTypeErrorbeforestorage.create_executionis ever called.Change
src/roma_dspy/api/execution_service.py:config_to_dict()fromroma_dspy.config.utils. That helper already handles bothROMAConfigand plainDictConfiginputs viaOmegaConf.to_container, and is the path the rest of the codebase uses for the same conversion (seeget_config_diffin the same module).model_dump/dict(config)ternary entirely — there is only one correct serialisation path.tests/unit/test_execution_service.py:mock_config_managerfixture to return a realOmegaConf.create(...)config instead of aMagicMockwith a pre-canned.model_dump. The mock-based test happily walked the (broken)model_dumpbranch in production code, masking the regression. Returning a real config exercises the actual serialisation path.Verification
python3 -m py_compileclean on both touched files.config_to_dict()is JSON-serializable, which is whatstorage.create_execution(PostgresStorage) ultimately needs.Out of scope
#74 also proposed adding a
free_tierconfig profile. I left that out —free_tieris not referenced in the current README,QUICKSTART.md, orCONFIGURATION.md(the examples all usegeneralandcrypto_agent), so the issue reporter's choice of"config_profile": "free_tier"would still fail profile lookup after this fix. The fix here ensures any valid profile name now serialises correctly; adding new profiles should be a separate scoped change.