-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[DOC][MINOR] Added step for dev env setup #4555
Conversation
Signed-off-by: Vilmos Agoston <vilmos.agoston@gmail.com>
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.
@vagoston Thanks for the PR! Left a comment!
CONTRIBUTING.rst
Outdated
@@ -149,6 +149,7 @@ If you plan on doing development and testing, you will also need to install the | |||
pip install -r dev-requirements.txt | |||
pip install -r test-requirements.txt | |||
pip install -e .[extras] # installs mlflow from current checkout | |||
pip install -e tests/resources/mlflow-test-plugin # installs an mlflow plugin for tests |
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.
pip install -e tests/resources/mlflow-test-plugin # installs an mlflow plugin for tests | |
pip install -e tests/resources/mlflow-test-plugin # installs `mlflow-test-plugin` that is required for running MLflow plugin tests |
Can we fix the comment to clarify why we need to install mlflow-test-plugin
?
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.
@harupy Cheers. The thing is that this plugin is required for a lot of tests, not just plugin tests, e.g.
mlflow/tests/deployments/test_cli.py
Line 15 in f7dda6e
def test_create(): |
I'll clarify the why part in the comment though.
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.
@vagoston You're right! Thanks for the update!
Signed-off-by: Vilmos Agoston <vilmos.agoston@gmail.com>
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.
LGTM
Signed-off-by: Vilmos Agoston vilmos.agoston@gmail.com
What changes are proposed in this pull request?
Minor change in setting up the dev env
How is this patch tested?
local tests pass
Release Notes
Is this a user-facing change?
What component(s), interfaces, languages, and integrations does this PR affect?
Components
area/docs
: MLflow documentation pagesrn/none
- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section