-
Notifications
You must be signed in to change notification settings - Fork 97
LCORE-2755: Model dumper #2096
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
LCORE-2755: Model dumper #2096
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| """Function to dump the schema of all data models into OpenAPI-compatible format.""" | ||
|
|
||
| import json | ||
|
|
||
| from pydantic.json_schema import models_json_schema | ||
|
|
||
| import models.compaction as models_compaction | ||
| from utils.json_schema_updater import recursive_update | ||
|
|
||
|
|
||
| def dump_models(filename: str) -> None: | ||
| """Dump the schema of all models into OpenAPI-compatible JSON file. | ||
|
|
||
| Parameters: | ||
| ---------- | ||
| - filename: str - name of file to export the schema to | ||
|
|
||
| Returns: | ||
| ------- | ||
| - None | ||
|
|
||
| Raises: | ||
| ------ | ||
| IOError: If the file cannot be written. | ||
| """ | ||
| with open(filename, "w", encoding="utf-8") as fout: | ||
| # retrieve the schema | ||
| _, schemas = models_json_schema( | ||
| [ | ||
| (model, "validation") | ||
| for model in [models_compaction.ConversationSummary] | ||
| ], | ||
| ref_template="#/components/schemas/{model}", | ||
| ) | ||
|
|
||
| # fix the schema | ||
| schemas = recursive_update(schemas) | ||
|
|
||
| # add all required metadata | ||
| openapi_schema = { | ||
| "openapi": "3.0.0", | ||
| "info": { | ||
| "title": "Lightspeed Core Stack", | ||
| "version": "0.3.0", | ||
| }, | ||
| "components": { | ||
| "schemas": schemas.get("$defs", {}), | ||
| }, | ||
| "paths": {}, | ||
| } | ||
| json.dump(openapi_schema, fout, indent=4) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| """Unit tests for utils/models_dumper module.""" | ||
|
|
||
| from json import load | ||
| from pathlib import Path | ||
|
|
||
| from utils.models_dumper import dump_models | ||
|
|
||
|
|
||
| def test_dump_models(tmpdir: Path) -> None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Use The 💚 Proposed fix-def test_dump_models(tmpdir: Path) -> None:
+def test_dump_models(tmp_path: Path) -> None:
"""Test that models can be dump into a JSON file.
An example of schema dump:
{
"openapi": "3.0.0",
"info": {
"title": "Lightspeed Core Stack",
"version": "0.3.0"
},
"components": {
"schemas": {
"ConversationSummary": {
"description": "A single compaction-produced summary chunk.\n\nAttributes:\n",
"properties": {
"summary_text": {
"description": "Natural-language summary produced by the ...",
"title": "Summary text",
"type": "string"
},
"summarized_through_turn": {
"description": "Running total of conversation items consumed by this ...",
"minimum": 0,
"title": "Summarized through turn",
"type": "integer"
},
"token_count": {
"description": "Number of tokens in summary_text.",
"minimum": 0,
"title": "Token count",
"type": "integer"
},
"created_at": {
"description": "ISO 8601 timestamp recording when this summary ...",
"title": "Created at",
"type": "string"
},
"model_used": {
"description": "Fully-qualified model identifier used for the ...",
"title": "Model used",
"type": "string"
}
},
"required": [
"summary_text",
"summarized_through_turn",
"token_count",
"created_at",
"model_used"
],
"title": "ConversationSummary",
"type": "object"
}
}
},
"paths": {}
}
"""
- filename = tmpdir / "foo.json"
+ filename = tmp_path / "foo.json"
dump_models(str(filename))
with open(filename, "r", encoding="utf-8") as fin:Also applies to: 67-70 🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai create issue
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="lightspeed-core/lightspeed-stack"
gh issue create \
--repo "$repo" \
--title "Use tmp_path instead of tmpdir fixture in test_models_dumper.py" \
--assignee "tisnik" \
--body "## Summary
In \`tests/unit/utils/test_models_dumper.py\`, the test \`test_dump_models\` (and the related fixture usage around lines 67-70) annotates the pytest fixture parameter as \`Path\` but receives it via the \`tmpdir\` fixture:
\`\`\`python
def test_dump_models(tmpdir: Path) -> None:
...
filename = tmpdir / \"foo.json\"
\`\`\`
## Rationale
The \`tmpdir\` fixture returns a \`py.path.local\` object, not a \`pathlib.Path\`, so the \`tmpdir: Path\` type annotation is incorrect and misleading. Modern pytest recommends using the \`tmp_path\` fixture, which returns an actual \`pathlib.Path\` object, keeping the type annotation accurate and the code aligned with current pytest best practices.
## Affected areas
- \`tests/unit/utils/test_models_dumper.py\` (function \`test_dump_models\`, also applies to lines 67-70)
## Suggested change
Replace the \`tmpdir\` fixture parameter with \`tmp_path\`, and update usages accordingly:
\`\`\`python
def test_dump_models(tmp_path: Path) -> None:
...
filename = tmp_path / \"foo.json\"
\`\`\`
## Acceptance criteria
- [ ] \`test_dump_models\` uses the \`tmp_path\` fixture instead of \`tmpdir\`.
- [ ] Type annotation \`Path\` matches the actual fixture return type.
- [ ] All usages within the test (including lines 67-70) are updated consistently.
## References
- PR: https://github.com/lightspeed-core/lightspeed-stack/pull/2096
- Review comment: https://github.com/lightspeed-core/lightspeed-stack/pull/2096#discussion_r3550097389
- Requested by: `@tisnik`"Length of output: 1953
It captures the |
||
| """Test that models can be dump into a JSON file. | ||
|
|
||
| An example of schema dump: | ||
| { | ||
| "openapi": "3.0.0", | ||
| "info": { | ||
| "title": "Lightspeed Core Stack", | ||
| "version": "0.3.0" | ||
| }, | ||
| "components": { | ||
| "schemas": { | ||
| "ConversationSummary": { | ||
| "description": "A single compaction-produced summary chunk.\n\nAttributes:\n", | ||
| "properties": { | ||
| "summary_text": { | ||
| "description": "Natural-language summary produced by the ...", | ||
| "title": "Summary text", | ||
| "type": "string" | ||
| }, | ||
| "summarized_through_turn": { | ||
| "description": "Running total of conversation items consumed by this ...", | ||
| "minimum": 0, | ||
| "title": "Summarized through turn", | ||
| "type": "integer" | ||
| }, | ||
| "token_count": { | ||
| "description": "Number of tokens in summary_text.", | ||
| "minimum": 0, | ||
| "title": "Token count", | ||
| "type": "integer" | ||
| }, | ||
| "created_at": { | ||
| "description": "ISO 8601 timestamp recording when this summary ...", | ||
| "title": "Created at", | ||
| "type": "string" | ||
| }, | ||
| "model_used": { | ||
| "description": "Fully-qualified model identifier used for the ...", | ||
| "title": "Model used", | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "summary_text", | ||
| "summarized_through_turn", | ||
| "token_count", | ||
| "created_at", | ||
| "model_used" | ||
| ], | ||
| "title": "ConversationSummary", | ||
| "type": "object" | ||
| } | ||
| } | ||
| }, | ||
| "paths": {} | ||
| } | ||
| """ | ||
| filename = tmpdir / "foo.json" | ||
| dump_models(str(filename)) | ||
|
|
||
| with open(filename, "r", encoding="utf-8") as fin: | ||
| # schema should be stored in JSON format | ||
| content = load(fin) | ||
| assert content is not None | ||
|
|
||
| # top-level keys test | ||
| keys = ("openapi", "info", "components", "paths") | ||
| for key in keys: | ||
| assert key in content | ||
|
|
||
| # components should be top-level node | ||
| components = content["components"] | ||
| assert components is not None | ||
|
|
||
| # schemas should be a node stored inside components node | ||
| assert "schemas" in components | ||
| schemas = components["schemas"] | ||
| assert schemas is not None | ||
|
|
||
| # list of schemas expected in a dump | ||
| expected_schemas = ("ConversationSummary",) | ||
| for expected_schema in expected_schemas: | ||
| assert expected_schema in schemas | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract shared helper to eliminate duplication with
schema_dumper.py.dump_modelsis nearly identical todump_schemainsrc/utils/schema_dumper.py(lines 11-47). The only differences are the model list and the docstring. This is a DRY violation — both functions generate an OpenAPI wrapper aroundmodels_json_schema+recursive_updatewith the same metadata.Consider extracting a shared helper that accepts a list of models and a filename, then having both
dump_schemaanddump_modelscall it.♻️ Proposed shared helper
🤖 Prompt for AI Agents
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.
@coderabbitai create issue
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.
🧩 Analysis chain
🏁 Script executed:
Length of output: 2211
@tisnikI've created the follow-up issue: #2099, assigned to you. It captures the DRY duplication betweendump_modelsanddump_schemaalong with the suggested shared-helper refactor.(=^..^=)