-
Notifications
You must be signed in to change notification settings - Fork 1.8k
doc: add Deprecation Policy section #5784
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
Conversation
Signed-off-by: junq <[email protected]>
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.
One reasonable scenario: you want to deprecate / make breaking change to some of the params of the LLM API, but you won't deprecate the whole LLM API/method/class
I don't think this scenario is explicitly covered in this deprecation policy but should be
WalkthroughA "Deprecation Policy" section was added to the README.md, outlining the process for deprecating APIs and tools, including notification methods, runtime warnings, a 3-month migration period, and subsequent removal aligned with semantic versioning. No changes were made to code or exported entities. Changes
Estimated code review effort1 (<30 minutes) Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
226-226
: Heading style violates markdownlint MD003
markdownlint-cli2
flags this line because the file currently mixes setext and ATX heading styles at the same level.
Either convert this heading to setext or configure the linter to accept mixed styles. For consistency with the majority of existing H2 sections, converting to setext is the minimal fix:-## Deprecation Policy +Deprecation Policy +==================
228-231
: Split long paragraph into bullets for scannabilityThe policy is excellent content-wise, but a single dense sentence hampers readability. A bullet list makes each guarantee immediately visible.
-Deprecation notices are communicated in the Release Notes. Deprecated API functions will have a statement in the source documenting when they were deprecated. Deprecated methods and classes will issue deprecation warnings at runtime, if they are used. TensorRT-LLM provides a 3-month migration period after the deprecation. APIs and tools continue to work during the migration period. After the migration period ends, APIs and tools are removed in a manner consistent with semantic versioning. +Deprecation notices appear in **Release Notes** and the source code. Deprecated entities: + +* include a comment indicating the version in which they were deprecated, +* emit a `DeprecationWarning` at runtime, +* remain functional for at least **3 months** to give users time to migrate. + +After the migration window, the APIs or tools are removed in accordance with semantic-versioning principles.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md
(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
226-226: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
Signed-off-by: junq <[email protected]>
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
226-226
: Prefer Setext‐style for level-2 headings if you want to silence markdownlintThe repo seems to have
MD003
enabled with the “setext” preference.
Switching## Deprecation Policy
to
Deprecation Policy ------------------
will satisfy the rule.
(Everything renders the same in GitHub Markdown; this is purely a style/lint matter.)
230-242
: Indent nested bullets four spaces to satisfyMD007
and keep lists crisp
markdownlint
is flagging the inner dash lists because they’re only indented by two spaces.
Four-space indentation is the conventional & linter-friendly offset for a sub-list under a numbered item.- - Deprecation notices are documented in the Release Notes. - - Deprecated APIs, methods, classes, or parameters include a statement in the source code indicating when they were deprecated. - - If used, deprecated methods, classes, or parameters issue runtime deprecation warnings. + - Deprecation notices are documented in the Release Notes. + - Deprecated APIs, methods, classes, or parameters include a statement in the source code indicating when they were deprecated. + - If used, deprecated methods, classes, or parameters issue runtime deprecation warnings. - - TensorRT-LLM provides a 3-month migration period after deprecation. - - During this period, deprecated APIs, tools, or parameters continue to work but trigger warnings. + - TensorRT-LLM provides a 3-month migration period after deprecation. + - During this period, deprecated APIs, tools, or parameters continue to work but trigger warnings. - - Full API/Method/Class Deprecation: The entire API/method/class is marked for removal. - - Partial Deprecation: If only specific parameters of an API/method are deprecated (e.g., param1 in LLM.generate(param1, param2)), the method itself remains functional, but the deprecated parameters will be removed in a future release. + - Full API/Method/Class Deprecation: the entire API/method/class is marked for removal. + - Partial Deprecation: if only specific parameters of an API/method are deprecated (e.g., `param1` in `LLM.generate(param1, param2)`), the method itself remains functional, but the deprecated parameters will be removed in a future release. - - After the 3-month migration period ends, deprecated APIs, tools, or parameters are removed in a manner consistent with semantic versioning (major version changes may include breaking removals). + - After the 3-month migration period ends, deprecated APIs, tools, or parameters are removed in a manner consistent with semantic versioning (major version changes may include breaking removals).This keeps rendering identical while quieting
MD004
/MD007
.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md
(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
226-226: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
231-231: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
231-231: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
232-232: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
232-232: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
233-233: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
233-233: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
235-235: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
235-235: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
236-236: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
236-236: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
238-238: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
238-238: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
239-239: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
239-239: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
241-241: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
241-241: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
@laikhtewari , updated the deprecation policy, and introduce "Partial Deprecation". Please have another look. |
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.
Nice addition of partial deprecation. Lgtm
/bot skip --comment "doc changes" |
PR_Github #12441 [ skip ] triggered by Bot |
PR_Github #12441 [ skip ] completed with state |
Signed-off-by: junq <[email protected]>
Signed-off-by: junq <[email protected]> Signed-off-by: Shreyas Misra <[email protected]>
Signed-off-by: junq <[email protected]> Signed-off-by: Ransiki Zhang <[email protected]>
Summary by CodeRabbit