Skip to content

Adjust/Improve pipeline to be compatible with more models#4

Draft
yuqiannemo wants to merge 13 commits intoXtra-Computing:devfrom
yuqiannemo:dna_dev/run_more_models
Draft

Adjust/Improve pipeline to be compatible with more models#4
yuqiannemo wants to merge 13 commits intoXtra-Computing:devfrom
yuqiannemo:dna_dev/run_more_models

Conversation

@yuqiannemo
Copy link
Copy Markdown
Contributor

@yuqiannemo yuqiannemo commented Mar 22, 2026

  1. Add more dependencies to support more models
  2. Protect sensitive info like openrouter API

@JerryLife
Copy link
Copy Markdown
Collaborator

Thanks for working on broader model compatibility here. Supporting more HF model families is a good direction, and the ModelLoader change is addressing a real misclassification issue.

That said, I don't think this is ready to merge as-is because the dependency changes would make the base package less portable.

Moving these packages into project.dependencies introduces hard platform requirements for every pip install llm-dna user, even if they never use those model families:

  • mlx and mlx-lm are Apple Silicon-specific — this will break installation on Linux/Windows, which is where most of our users run the package
  • optimum, compressed-tensors, etc. are architecture-specific/transitive runtime deps rather than core package requirements

I think the right direction is:

  • Keep the base install minimal
  • Move platform/model-specific packages into optional dependency groups (e.g. apple, quantization)
  • Document which extras are needed for which model families

On ModelLoader, the current fix works for openai/gpt-oss, but it's brittle because it carves out exceptions from a broad openai/gpt- rule — every new HuggingFace model under the openai/ namespace would need another exclusion. Instead, I'd suggest tightening the OpenRouter prefixes to be more specific:

openrouter_prefixes = [
    "openrouter/",
    "openrouter:",
    "anthropic/claude-",
    "deepseek/",
    "openai/gpt-3",
    "openai/gpt-4",
    "google/gemini-",
    "x-ai/grok-",
    "cohere/command",
    "perplexity/",
]

This way openai/gpt-3.5-turbo and openai/gpt-4o still route to OpenRouter, but openai/gpt-oss-* naturally falls through to HuggingFace without needing any exclusion logic.

One smaller process note: the PR description looks incomplete (2. is empty). Please expand that before merge so the packaging/dependency rationale is clear.

Net: I like the goal of the PR, but the dependency strategy needs to change before this can be merged.

@yuqiannemo
Copy link
Copy Markdown
Contributor Author

Thanks for working on broader model compatibility here. Supporting more HF model families is a good direction, and the ModelLoader change is addressing a real misclassification issue.

That said, I don't think this is ready to merge as-is because the dependency changes would make the base package less portable.

Moving these packages into project.dependencies introduces hard platform requirements for every pip install llm-dna user, even if they never use those model families:

  • mlx and mlx-lm are Apple Silicon-specific — this will break installation on Linux/Windows, which is where most of our users run the package
  • optimum, compressed-tensors, etc. are architecture-specific/transitive runtime deps rather than core package requirements

I think the right direction is:

  • Keep the base install minimal
  • Move platform/model-specific packages into optional dependency groups (e.g. apple, quantization)
  • Document which extras are needed for which model families

On ModelLoader, the current fix works for openai/gpt-oss, but it's brittle because it carves out exceptions from a broad openai/gpt- rule — every new HuggingFace model under the openai/ namespace would need another exclusion. Instead, I'd suggest tightening the OpenRouter prefixes to be more specific:

openrouter_prefixes = [
    "openrouter/",
    "openrouter:",
    "anthropic/claude-",
    "deepseek/",
    "openai/gpt-3",
    "openai/gpt-4",
    "google/gemini-",
    "x-ai/grok-",
    "cohere/command",
    "perplexity/",
]

This way openai/gpt-3.5-turbo and openai/gpt-4o still route to OpenRouter, but openai/gpt-oss-* naturally falls through to HuggingFace without needing any exclusion logic.

One smaller process note: the PR description looks incomplete (2. is empty). Please expand that before merge so the packaging/dependency rationale is clear.

Net: I like the goal of the PR, but the dependency strategy needs to change before this can be merged.

Fixed

Copy link
Copy Markdown
Collaborator

@JerryLife JerryLife left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! The optional extras and sensitive info protection are great additions. A few issues to address before this is merge-ready:

Correctness

  1. Hardcoded relative paths will break when installed as a package

    • ModelLoader.py uses Path(__file__).parents[3] / "configs" / "openrouter_llm_list.jsonl" — this assumes a source-tree layout. When installed via pip install llm-dna, the configs/ directory won't exist at that relative path. Consider using importlib.resources or including the file as package data.
    • Same issue in cli.py with Path(__file__).parents[2] / ".env" — this path won't resolve correctly in an installed package. The .env loading should probably just use load_dotenv() (which searches CWD and parent dirs) without the hardcoded project root path.
  2. Missing openrouter_llm_list.jsonl

    • The diff references configs/openrouter_llm_list.jsonl but the file doesn't appear in the diff. Is it committed on the branch? If so, it also needs to be included as package data in pyproject.toml to be available at install time.
  3. Redundant inference in single-model response caching (api.py)

    • The new block at L645-672 calls _generate_responses_for_model() again even though inference was already performed above for DNA extraction. This effectively doubles the computation cost for single-model mode. The responses from the earlier inference step should be reused instead of regenerated.

Improvements

  1. [full] extras should reference sub-extras
    Instead of duplicating all packages:

    full = [
        "llm-dna[apple]",
        "llm-dna[quantization]",
        "llm-dna[model_families]",
    ]

    This avoids needing to keep version pins in sync across two places.

  2. Platform-specific markers
    mlx / mlx-lm only work on macOS Apple Silicon, and mamba-ssm requires CUDA. Consider adding environment markers:

    apple = [
        "mlx>=0.10.0; sys_platform == 'darwin'",
    ]
  3. No tests for new model detection logic
    The OpenRouter model detection via JSONL lookup is a significant behavioral change — a unit test would help prevent regressions.

Overall the direction is solid. Happy to help if you have questions on any of these points!

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.

2 participants