Skip to content
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

epic: Semantic naming for Cortex Engines? #1168

Closed
Tracked by #1072
dan-menlo opened this issue Sep 9, 2024 · 8 comments · Fixed by #1406
Closed
Tracked by #1072

epic: Semantic naming for Cortex Engines? #1168

dan-menlo opened this issue Sep 9, 2024 · 8 comments · Fixed by #1406
Assignees
Labels
category: engine management Related to engine abstraction P0: critical Mission critical
Milestone

Comments

@dan-menlo
Copy link
Contributor

dan-menlo commented Sep 9, 2024

Goal / Summary of discussion

  • We should have more semantic naming format for Cortex engines
  • We should go with how the upstream authors name it, i.e.
    • onnxruntime
    • llama-cpp
    • tensorrt-llm
# Name Supported Formats Version Status
1 onnxruntime ONNX 0.0.1 Incompatible
2 llama-cpp GGUF 0.0.1 Ready
3 tensorrt-llm TensorRT Engines 0.0.1 Incompatible

Out of scope - renaming repos?

For the Repos, I recommend we add an *-engine suffix to differentiate us from the upstream library:

Old Repo name New Repo name
janhq/cortex.tensorrt-llm janhq/tensorrt-llm-engine
janhq/cortex.llamacpp janhq/llamacpp-engine
janhq/cortex.onnx janhq/onnxruntime-engine

Tasklist

@dan-menlo dan-menlo added this to Menlo Sep 9, 2024
@dan-menlo dan-menlo converted this from a draft issue Sep 9, 2024
@dan-menlo dan-menlo moved this to Scheduled in Menlo Sep 9, 2024
@freelerobot freelerobot added the category: engine management Related to engine abstraction label Sep 10, 2024
@namchuai
Copy link
Contributor

IMO, maybe keep it as llamacpp, onnx and tensorrt

@dan-menlo
Copy link
Contributor Author

dan-menlo commented Sep 15, 2024

@tikikun @nguyenhoangthuan99 Can I check: if we are using "engine names", should it be:

  • llamacpp (format: gguf)
  • onnxruntime (format: onnx)
  • tensorrt-llm? (format: tensorrt engines)

@freelerobot
Copy link
Contributor

freelerobot commented Sep 23, 2024

Lack of decision causing issues here: #1283

Gentle bump @nguyenhoangthuan99 @vansangpfiev to come to an agreement on this.

Option 1:

llama-cpp
onnx-runtime
tensorrt-llm

Option 2:

llamacpp
onnxruntime
tensortllm

Current (inconsistent):

❯ cortex-nightly engines list
+---+--------------+-------------------+---------+--------------+
| # | Name         | Supported Formats | Version | Status       |
+---+--------------+-------------------+---------+--------------+
| 1 | ONNXRuntime  | ONNX              | 0.0.1   | Incompatible |
+---+--------------+-------------------+---------+--------------+
| 2 | llama.cpp    | GGUF              | 0.0.1   | Ready        |
+---+--------------+-------------------+---------+--------------+
| 3 | TensorRT-LLM | TensorRT Engines  | 0.0.1   | Incompatible |
+---+--------------+-------------------+---------+--------------+

@freelerobot freelerobot moved this from Scheduled to Planning in Menlo Sep 23, 2024
@freelerobot freelerobot added this to the v0.1.0 milestone Sep 23, 2024
@dan-menlo dan-menlo moved this from Planning to Triage in Menlo Sep 29, 2024
@nguyenhoangthuan99 nguyenhoangthuan99 moved this from Investigating to In Progress in Menlo Oct 1, 2024
@nguyenhoangthuan99 nguyenhoangthuan99 moved this from In Progress to Investigating in Menlo Oct 1, 2024
@gabrielle-ong
Copy link
Contributor

@nguyenhoangthuan99 @vansangpfiev adding this as a priority to decide for consistency before the release

@nguyenhoangthuan99
Copy link
Contributor

nguyenhoangthuan99 commented Oct 2, 2024

Related issue: #1283
To choose the best option for both user experience and development experience, let's analyze the two proposed options and the current state:

  • Option 1: llama-cpp, onnx-runtime, tensorrt-llm
  • Option 2: llamacpp, onnxruntime, tensortllm
  • Current: llama.cpp, ONNXRuntime, TensorRT-LLM

Analysis:

  1. Consistency: Option 2 is the most consistent, using all lowercase and no hyphens. This can be beneficial for both users and developers, as it reduces cognitive load and potential for errors in typing or remembering names.

  2. Readability: Option 1 is more readable due to the use of hyphens, which can help in quickly distinguishing between words. However, this advantage is minor.

  3. Familiarity: The current naming uses the official names of the projects (llama.cpp, ONNXRuntime, TensorRT-LLM). This might be more familiar to users who know these projects.

  4. Consistency with current implementation: Option 1 is closer to the current implementation, which might make the transition easier for existing users.

I prefer Option 1 to update our engine naming convention to llama-cpp, onnx-runtime, and tensorrt-llm. This change brings consistency across our engine names while maintaining their familiarity. The new format improves readability and aligns with common practices in the open-source community, enhancing both user experience and code maintainability. This standardized approach will also help us smoothly integrate any future engines or technologies.

The output of terminal when list engines:

# Name Supported Formats Version Status
1 onnx-runtime ONNX 0.0.1 Incompatible
2 llama-cpp GGUF 0.0.1 Ready
3 tensorrt-llm TensorRT Engines 0.0.1 Incompatible

cc @vansangpfiev @namchuai @dan-homebrew @0xSage

@dan-menlo
Copy link
Contributor Author

dan-menlo commented Oct 3, 2024

@nguyenhoangthuan99 @vansangpfiev @namchuai I think we should go with how the upstream authors name it, i.e.

  • onnxruntime
  • llama-cpp
  • tensorrt-llm

So essentially how @nguyenhoangthuan99 names them, with just a change on onnxruntime.

# Name Supported Formats Version Status
1 onnxruntime ONNX 0.0.1 Incompatible
2 llama-cpp GGUF 0.0.1 Ready
3 tensorrt-llm TensorRT Engines 0.0.1 Incompatible

For the Repos, I recommend we add an *-engine suffix to differentiate us from the upstream library:

Old Repo name New Repo name
janhq/cortex.tensorrt-llm janhq/tensorrt-llm-engine
janhq/cortex.llamacpp janhq/llamacpp-engine
janhq/cortex.onnx janhq/onnxruntime-engine

@dan-menlo dan-menlo moved this from Investigating to Scheduled in Menlo Oct 3, 2024
@vansangpfiev vansangpfiev mentioned this issue Oct 3, 2024
3 tasks
@gabrielle-ong gabrielle-ong moved this from Scheduled to In Progress in Menlo Oct 4, 2024
@gabrielle-ong
Copy link
Contributor

gabrielle-ong commented Oct 5, 2024

Marking as done! Engines renamed for consistent experience
Image

Out of scope: Renaming Repos task - moving to a discussion for Sprint 22

@github-project-automation github-project-automation bot moved this from In Progress to Review + QA in Menlo Oct 5, 2024
@gabrielle-ong gabrielle-ong moved this from Review + QA to Completed in Menlo Oct 5, 2024
@hiento09
Copy link
Contributor

Reopen this epic, we have not rename upstream engines repo yet

@hiento09 hiento09 reopened this Oct 22, 2024
@dan-menlo dan-menlo changed the title decision: Semantic naming for Cortex Engines? epic: Semantic naming for Cortex Engines? Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: engine management Related to engine abstraction P0: critical Mission critical
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

8 participants