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

docs: Simplify PR templates #7753

Open
wants to merge 3,442 commits into
base: main
Choose a base branch
from
Open

docs: Simplify PR templates #7753

wants to merge 3,442 commits into from

Conversation

yinggeh
Copy link
Contributor

@yinggeh yinggeh commented Oct 29, 2024

What does the PR do?

Internal: remove CI Pipeline ID and commit type sections. Optional GitHub issue.
External: remove commit type section.

Checklist

  • PR title reflects the change and is of format <commit_type>: <Title>
  • Changes are described in the pull request.
  • Related issues are referenced.
  • Populated github labels field
  • Added test plan and verified test passes.
  • Verified that the PR passes existing CI.
  • Verified copyright is correct on all changed files.
  • Added succinct git squash message before merging ref.
  • All template sections are filled out.
  • Optional: Additional screenshots for behavior/output changes with before/after.

Commit Type:

Check the conventional commit type
box here and add the label to the github PR.

  • docs

Related PRs:

Where should the reviewer start?

Test plan:

  • CI Pipeline ID:

Caveats:

Background

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

  • closes GitHub issue: #xxx

mc-nv and others added 30 commits December 11, 2023 01:54
* Mlflow plugin fix
* Unify iGPU test build with x86 ARM

* adding TRITON_IGPU_BUILD to core build definition; adding logic to skip caffe2plan test if TRITON_IGPU_BUILD=1

* re-organizing some copies in Dockerfile.QA to fix igpu devel build

* Pre-commit fix

---------

Co-authored-by: kyle <[email protected]>
* adding default value for TRITON_IGPU_BUILD=OFF

* fix newline

---------

Co-authored-by: kyle <[email protected]>
* Add test case for decoupled model raising exception

* Remove unused import

* Address comment
* vLLM Benchmarking Test
#6639)

* Add ability to configure GRPC max connection age and max connection age grace
* Allow pass GRPC connection age args when they are set from command
----------
Co-authored-by: Katherine Yang <[email protected]>
…d test (#6713)

* Modify HTTP frontend to return error code reflecting Triton error

* Add test for dedicated HTTP error. Releax existing test on HTTP code

* Address comment. Fix copy right
* Update README and versions for 23.12 branch

* Bring back the README (#6671)

* Bring back the README

* main -> r23.12

* Remove L0_libtorch_nvfuser (#6674)

* iGPU build refactor (#6684)

* Fix iGPU CMakeFile tags (#6695) (#6698)

* Unify iGPU test build with x86 ARM

* adding TRITON_IGPU_BUILD to core build definition; adding logic to skip caffe2plan test if TRITON_IGPU_BUILD=1

* re-organizing some copies in Dockerfile.QA to fix igpu devel build

* Pre-commit fix

---------

Co-authored-by: kyle <[email protected]>

* Update windows Dockerfile versions (#6672)

Changing version to the latest one

Co-authored-by: Misha Chornyi <[email protected]>

* Remove README banner (#6719)

* Update README

---------

Co-authored-by: tanmayv25 <[email protected]>
Co-authored-by: Jacky <[email protected]>
Co-authored-by: kyle <[email protected]>
* testing apprroach with pre-built image

* Build TensorRT-LLM

* Disable Triton Build

* Remove file

* Update config

* Changet PATH variables

* Update path

* Update configuration for CMake

* Getting back TRITON_BUILD flag

* REvert missing files creation

* Update configuration for the PyTorch installation

* Update configuration for docker

* Change the location

* Update configuration

* update config

* Set CMake version to 3.27.7

* Fix double slash typo

* remove unused strings

* restore typo (#6680)

* remove old line

* fix line indentation

* Update LD_LIBRARY_PATH for TensorRT-LLM

* Addign TRT llm changes

* remove TRT-LLM container from bhte argument list

* Update indentation
* Update RE2 package location

* Use only 1 parallel thread for build

* Revert "Use only 1 parallel thread for build"

This reverts commit 93eab3a.
* Add testing for zero tensors in PyTorch backend

* Fix up

* Review edit
* Do not fail test on insufficient hardware concurrency

* Track instead of fail test if cannot replicate load while async unload

* Add some TODOs for the sub-test
* Simplify cmake install command

* Fix up

* Review comment
* Add cmdline option to set model load retry. Add test

* Fix copyright

* Minor change on testing model

* Remove unused import
- Extend L0_storage_S3 test timeout
* Patch L0_model_config with runtime

* Add L0_pytorch_python_runtime

* Update expected runtime field

* Add test for escaping runtime

* Add comments on unit test imports

* Add invalid runtime test

* User to build PyTorch env

* Update copyright
* Test case

* Update metrics.md

* Fix alert

* Add copyright

* Update test

* Improve pinned_memory_metrics_test.py

* Update qa/L0_metrics/pinned_memory_metrics_test.py

Co-authored-by: Ryan McCormick <[email protected]>

* Update pinned_memory_metrics_test.py

---------

Co-authored-by: Ryan McCormick <[email protected]>
Added support for OTel context propagation

---------

Co-authored-by: Markus Hennerbichler <[email protected]>
Co-authored-by: Ryan McCormick <[email protected]>
This validates the change made to ../core wrt how model configuration mtime is handled.
@yinggeh yinggeh added the PR: docs Documentation only changes label Oct 29, 2024
@yinggeh yinggeh self-assigned this Oct 29, 2024
@yinggeh yinggeh changed the title docs: Update pull_request_template_internal_contrib.md docs: Simplify PR templates Oct 29, 2024
- [ ] Added [test plan](#test-plan) and verified test passes.
- [ ] Verified that the PR passes existing CI.
- [ ] Verified copyright is correct on all changed files.
- [ ] Added _succinct_ git squash message before merging [ref](https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html).
- [ ] All template sections are filled out.
- [ ] Optional: Related issues are referenced.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be optional. It's useful for cherry picks to know of related PRs, and if the PR closes an issue, we should mark it as closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's optional because most PRs do not related to a GitHub issue.

Copy link
Contributor

@jbkyang-nvi jbkyang-nvi Oct 29, 2024

Choose a reason for hiding this comment

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

when I created this PR, I conflated the word "issue". I agree that only sometimes the PR is related to a Github issue. Can you add a checkbox that says:

Suggested change
- [ ] Optional: Related issues are referenced.
- [ ] Linked related PRs, if any.
- [ ] Optional: Related issues are referenced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already a "Related PRs" section.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think "related issue" should be a dedicated section, GitHub should be able to link and close the issue automatically if the specified format is followed: https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-->

Copy link
Contributor

Choose a reason for hiding this comment

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

will updating copyright here produce a big blob of text in the PR template? If yes, I suggest we remove it

Copy link
Contributor Author

@yinggeh yinggeh Oct 29, 2024

Choose a reason for hiding this comment

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

Not sure if we can merge without passing all pre-commit checks. cc @rmccorm4

Copy link
Contributor

@jbkyang-nvi jbkyang-nvi Oct 29, 2024

Choose a reason for hiding this comment

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

I would argue this piece of code specifically, does not need a copyright. If you are concerned with the pre-commit check for copyright being not accurate, please link the copyright checking script: https://github.com/triton-inference-server/server/blob/main/qa/common/check_copyright.py. You can see that the Pull Request document is excluded from the check as well as general .md files

Copy link
Contributor Author

@yinggeh yinggeh Oct 30, 2024

Choose a reason for hiding this comment

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

It's the recent PR that added add_copyright.py to the pre-commit hook.
We might want to skip some files and paths like we do in check_copyright.py.
See #7666

#### What does the PR do?
<!-- Describe your pull request here. Please read the text below the line, and make sure you follow the checklist.-->

#### Checklist
- [ ] PR title reflects the change and is of format `<commit_type>: <Title>`
- [ ] PR title reflects the change and is of format `<commit_type>: <Title>`. Check the [conventional commit type](https://github.com/angular/angular/blob/22b96b9/CONTRIBUTING.md#type).
Copy link
Contributor

Choose a reason for hiding this comment

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

I would want to have a list of commonly used types ("i.e. feat, fix, doc") and mention that the link points to more types.

@pvijayakrish pvijayakrish force-pushed the yinggeh-update-pr-template branch from db56608 to dd41b92 Compare January 15, 2025 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: docs Documentation only changes
Development

Successfully merging this pull request may close these issues.