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

Issue policy and template tweaks #2739

Merged
merged 21 commits into from
Apr 24, 2020
Merged

Conversation

dbczumar
Copy link
Collaborator

What changes are proposed in this pull request?

  • Revamp "feature request" template to prompt filers for specific motivation questions and high-level implementation details (if possible)
  • Revamp Issue Policy to address the lifecycle for each issue type
  • Add "Willingness to contribute" sections to the feature request, bug report, and documentation fix templates

How is this patch tested?

Manually - I recommend viewing the associated files in a markdown renderer (e.g., open the file in the GitHub file browser).

Release Notes

Is this a user-facing change?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release notes for MLflow users.

(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)

What component(s) does this PR affect?

  • UI
  • CLI
  • API
  • REST-API
  • Examples
  • Docs
  • Tracking
  • Projects
  • Artifacts
  • Models
  • Model Registry
  • Scoring
  • Serving
  • R
  • Java
  • Python

How should the PR be classified in the release notes? Choose one:

  • rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
  • rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
  • rn/feature - A new user-facing feature worth mentioning in the release notes
  • rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
  • rn/documentation - A user-facing documentation change worth mentioning in the release notes

Comment on lines 56 to 57
- [ ] Scoring
- [ ] Serving
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sueann Can we dedupe Scoring and Serving here and in the pull request template?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also API and REST-API? Dunno how a user would choose between those.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good - i think we want to just keep "Scoring"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the input! I've consolidated Serving and Scoring to just Scoring. I've consolidated API and REST-API as just API.


Please fill in this template and do not delete it unless you are sure your issue is outside its scope.
**Please fill in this bug report template to ensure a thorough response.**
Copy link
Contributor

@aarondav aarondav Apr 22, 2020

Choose a reason for hiding this comment

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

Do I want a thorough response? Quick? Some other word?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've changed this to timely and thorough. More than happy to iterate here though!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

^ I applied these changes to the PR template as well


- [ ] Yes. I can contribute a fix for this bug independently.
- [ ] Yes. I would be willing to contribute a fix for this bug with guidance from the MLflow community.
- [ ] No. **Explanation:**
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we can leave out the Explanation bit. Just having "No" here is already 🤨

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!


- [ ] Yes. I think this feature can be introduced as an MLflow Plugin.
- [ ] No. It does not make sense to structure this feature as an MLflow Plugin. **Explanation:**
- [ ] No. I don’t think MLflow Plugins currently supports this type of feature. **Explanation:**
Copy link
Contributor

Choose a reason for hiding this comment

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

s/supports/support/?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done! Thanks!

@codecov-io
Copy link

Codecov Report

Merging #2739 into master will increase coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2739      +/-   ##
==========================================
+ Coverage   85.04%   85.23%   +0.19%     
==========================================
  Files          20       20              
  Lines        1050     1050              
==========================================
+ Hits          893      895       +2     
+ Misses        157      155       -2     
Impacted Files Coverage Δ
R/tracking-server.R 100.00% <0.00%> (+3.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2300d8e...de253ea. Read the comment docs.

@dbczumar
Copy link
Collaborator Author

@aarondav @smurching @mateiz In response to feedback that the feature request template is too lengthy and technical, I've removed the sections related to plugins and implementation impact from the template. Instead, I've updated the CONTRIBUTING.rst document to provide guidelines about feature / patch design and information about structuring features as plugins, as well as an overview of the contribution process. The feature request template explicitly encourages users to consult the contributing guide. Another round of reviews would be greatly appreciated!

MLflow committers actively triage and respond to GitHub issues. In general, we recommend waiting
for feebdack from an MLflow committer or community member before proceeding to implement a feature
or patch. This is particularly important for
`significant changes <https://github.com/mlflow/mlflow/blob/master/CONTRIBUTING.rst#write-designs-for-significant-changes>`_.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried using RST references (:ref:) to link to sections within this document. Unfortunately, GitHub doesn't seem to render RST references properly (see isaacs/github#892), so I gave this a shot instead.

## Which MLflow component(s) does this feature affect?

- [ ] UI
- [ ] CLI
Copy link
Contributor

Choose a reason for hiding this comment

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

Some people might not know what CLI stands for so maybe spell it out

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed this to read Command Line Interface and also made the corresponding change to the pull request template. Thanks for pointing this out!

Copy link
Collaborator

@smurching smurching left a comment

Choose a reason for hiding this comment

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

One nit, but otherwise LGTM thanks @dbczumar!

@dbczumar dbczumar changed the title [WIP] Issue policy and template tweaks Issue policy and template tweaks Apr 23, 2020
Co-Authored-By: Siddharth Murching <smurching@gmail.com>
@dbczumar dbczumar merged commit 8eb4da0 into mlflow:master Apr 24, 2020
@smurching smurching added the rn/none List under Small Changes in Changelogs. label Jun 18, 2020
avflor pushed a commit to avflor/mlflow that referenced this pull request Aug 22, 2020
* Initial

* Tweaks

* Spacing

* Bug fix

* Lang

* Unbold

* Doc issues

* Tweak

* Installation

* Issue policy

* Ordering

* Update guidelines

* Address feedback

* Address

* Spacing

* Progress

* Format

* template update

* Address comments

* Whitespace fixes for contributing guide

* Update .github/ISSUE_TEMPLATE/feature_request_template.md

Co-Authored-By: Siddharth Murching <smurching@gmail.com>

Co-authored-by: Siddharth Murching <smurching@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rn/none List under Small Changes in Changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants