Skip to content

chore: Add KEP to the supported PR titles #66

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
+2 −0
Closed

chore: Add KEP to the supported PR titles #66

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4dabfc4
Add Proposal to the supported PR titles
szaher Aug 22, 2025
8bb4d8d
Use KEP instead of proposal
szaher Aug 22, 2025
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
2 changes: 2 additions & 0 deletions .github/workflows/check-pr-title.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,13 @@ jobs:
fix
feat
revert
KEP
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need it? I think we should use feat: KEP-NN

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say yes, KEP is different from features. features are new features that we introduce to the sdk but KEP is a proposal or enhancement proposal.


scopes: |
ci
docs
examples
proposals
Copy link
Contributor

Choose a reason for hiding this comment

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

We can just use feat(docs): KEP-NN for that

Copy link
Member Author

Choose a reason for hiding this comment

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

proposal are not really docs... wDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I kind of agree with @kramaranya, we can use (docs) scope for KEPs. Otherwise, we can just to feat: KEP-nn, since scope is not required.
I would like us to keep feat, fix in the title name since it is easier to group them in CHANGELOG later.

Copy link
Member Author

Choose a reason for hiding this comment

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

if we use feat it will be confusing to users or whoever reads the changelog or release notes. Users would assume this feature is already implemented as the release notes mentions it as one of the PRs.

If we add KEP it clearly signals this is a TODO or planned feature...

feat: Local Execution >>> contains KEP
feat: Local Execution >>> Actual implementation

cc: @andreyvelich @kramaranya

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but we can group them in the dedicated subsection in the CHANGELOG, like we did here:
https://github.com/kubeflow/trainer/blob/master/CHANGELOG.md#llm-trainer-v2

Copy link
Member

Choose a reason for hiding this comment

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

FYI, KFP also uses docs prefix for KEPs: kubeflow/pipelines#11551

Copy link
Member Author

@szaher szaher Aug 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If we add the KEP type I think it will be consistent with the following.
image
which I believe is cleaner.

With KEP type
KEP 2: Local execution
feat: Local execution

Without KEP type (Current Behaviour)
docs: KEP-2 Local exection
feat: Local execution

Which one do you think is cleaner?

I believe we can group both or leave both in the release notes in the respective sections

Copy link
Member

Choose a reason for hiding this comment

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

I suggest that we use feat: and fix: prefix, and we add the KEP number after.
E.g.:

feat: KEP-2 - Add local backend for KF Trainer
fix: KEP-2 - Update the runtime return type
chore: KEP-2 Add unit tests for utils

Since that will be simpler to group fixes, chores, and features dedicated to the particular KEP.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @andreyvelich, that would be much cleaner. Also, if we add a KEP prefix, it could confuse contributors, and they might end up using both options:

feat: KEP-NN - ...
KEP-NN: ...

So I think it’s better to stick with one clear format to keep it consistent

scripts
test
trainer
Expand Down