Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 52 additions & 18 deletions ddss/main.py
Original file line number Diff line number Diff line change
@@ -1,24 +1,38 @@
import sys
import asyncio
import tempfile
import pathlib
from typing import Annotated, Optional
import tyro
from .orm import initialize_database
from .ds import main as ds
from .egg import main as egg
from .input import main as input
from .output import main as output
from .load import main as load
from .dump import main as dump

component_map = {
"ds": ds,
"egg": egg,
"input": input,
"output": output,
"load": load,
"dump": dump,
}


async def main(addr):
async def run(addr: str, components: list[str]) -> None:
Copy link

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

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

The run() function is missing type annotations for its parameters. The diff shows type hints in line 24 (async def run(addr: str, components: list[str]) -> None:), but the actual implementation uses async def run(addr, components) without type annotations. Adding type annotations would improve code clarity and enable better static type checking, which is especially important since the rest of the code is being migrated to use type-annotated interfaces with tyro.

Copilot uses AI. Check for mistakes.
engine, session = await initialize_database(addr)

try:
try:
coroutines = [component_map[component](addr, engine, session) for component in components]
except KeyError as e:
print(f"error: unsupported component {e}")
Copy link

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

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

The error message format is inconsistent. When a component is invalid (line 31), the error prints error: unsupported component {e}, which will display the KeyError exception object (e.g., KeyError('invalid_component')). However, when the database is unsupported (line 83), it prints the actual address value.

For consistency and clarity, line 31 should extract and display the component name from the KeyError, such as: print(f"error: unsupported component: {component}") where component is the invalid component name.

Suggested change
print(f"error: unsupported component {e}")
component = e.args[0] if e.args else e
print(f"error: unsupported component: {component}")

Copilot uses AI. Check for mistakes.
raise asyncio.CancelledError()

await asyncio.wait(
[
asyncio.create_task(ds(addr, engine, session)),
asyncio.create_task(egg(addr, engine, session)),
asyncio.create_task(input(addr, engine, session)),
asyncio.create_task(output(addr, engine, session)),
],
[asyncio.create_task(coro) for coro in coroutines],
return_when=asyncio.FIRST_COMPLETED,
)
except asyncio.CancelledError:
Expand All @@ -35,26 +49,46 @@ async def main(addr):
}


def cli():
if len(sys.argv) == 1:
def main(
addr: Annotated[
Optional[str],
tyro.conf.arg(
aliases=["-a"],
help="Database address URL. If not provided, uses a temporary SQLite database.",
),
] = None,
component: Annotated[
list[str],
tyro.conf.arg(
aliases=["-c"],
help="Components to run.",
Copy link

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

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

The default value for the component parameter is missing "load" and "dump" components that are defined in component_map (lines 11-21). According to the PR description, it claims to preserve existing behavior, but the original code only ran ds, egg, input, and output. However, since load and dump are now added to the component_map, users might expect them to be available. The inconsistency is that these components exist in the map but aren't mentioned in the help text or included as a clear option.

Consider either:

  1. Documenting which components are available (e.g., in the help text)
  2. Validating component names against the component_map keys early with a clear error message listing valid options
Suggested change
help="Components to run.",
help="Components to run. Available: ds, egg, input, output, load, dump.",

Copilot uses AI. Check for mistakes.
),
] = ["input", "output", "ds", "egg"],
) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

缺少一个 component: list[str] 的功能, 默认是 ["input", "output", "ds", "egg"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in commit 5c11325. Added component: list[str] parameter with default value ["input", "output", "ds", "egg"]. Usage:

# Run all components (default)
ddss

# Run specific components
ddss --component input output

# Run only ds and egg
ddss --component ds egg

"""DDSS - Distributed Deductive System Sorts

Run DDSS with an interactive deductive environment.
"""
if addr is None:
tmpdir = tempfile.TemporaryDirectory()
path = pathlib.Path(tmpdir.name) / "ddss.db"
addr = f"sqlite:///{path.as_posix()}"
Comment on lines 73 to 75
Copy link

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

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

The tmpdir variable is created as a TemporaryDirectory object but is never stored or cleaned up properly. This means the temporary directory will be deleted when tmpdir goes out of scope at the end of the main() function, potentially before the async run() function completes and while the database is still in use. This could cause the database file to be deleted while the application is running.

To fix this, the tmpdir should be kept alive for the entire duration of the async operation, or the cleanup should be handled explicitly in the async context.

Copilot uses AI. Check for mistakes.
elif len(sys.argv) == 2 and sys.argv[1] not in ["--help", "-help", "-h", "/help", "/h", "/?"]:
addr = sys.argv[1]
else:
print(f"Usage: {sys.argv[0]} [<database-addr>]")
sys.exit(1)

for key, value in sqlalchemy_driver.items():
if addr.startswith(f"{key}://"):
addr = addr.replace(f"{key}://", f"{key}+{value}://")
if addr.startswith(f"{key}+{value}://"):
break
else:
print(f"Unsupported database address: {addr}")
sys.exit(1)
print(f"error: unsupported database: {addr}")
raise SystemExit(1)

print(f"addr: {addr}")
asyncio.run(main(addr))
asyncio.run(run(addr, component))
Comment on lines +60 to +87
Copy link

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

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

The run() function signature in the diff shows parameter types addr: str, components: list[str], but when calling it at line 87, the function is invoked with component (singular) instead of components (plural). However, looking at line 24 in the actual code, the function definition uses components (plural). This appears to be correctly implemented in the actual code, but there's an inconsistency between the parameter name in main() (line 60: component) and in run() (line 24: components).

For consistency and clarity, consider using the same parameter name in both functions, preferably components (plural) since it's a list of multiple components.

Copilot uses AI. Check for mistakes.


def cli():
tyro.cli(main)


if __name__ == "__main__":
Expand Down
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ dependencies = [
"apyds-egg~=0.0.11",
"prompt-toolkit~=3.0.52",
"sqlalchemy[aiosqlite,aiomysql,postgresql-asyncpg]~=2.0.45",
"tyro~=1.0.3",
Copy link
Member

Choose a reason for hiding this comment

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

记得重新uv lock一下

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in commit 5c11325. Ran uv lock to update the lockfile with tyro 1.0.3 and related dependencies.

]

[project.urls]
Expand Down
43 changes: 40 additions & 3 deletions uv.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.