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

Update UI #5577

Merged
merged 8 commits into from
Apr 5, 2022
Merged

Update UI #5577

merged 8 commits into from
Apr 5, 2022

Conversation

harupy
Copy link
Member

@harupy harupy commented Apr 1, 2022

Signed-off-by: harupy 17039389+harupy@users.noreply.github.com

What changes are proposed in this pull request?

Update MLflow UI.

How is this patch tested?

  • JS unit tests
  • Generate run data by running the script below and manually check the UI works fine.
import os
import mlflow
import random

from sklearn.linear_model import LinearRegression


def log():
    for i in range(50):

        with mlflow.start_run():
            mlflow.log_params({"p1": random.random(), "p2": random.random()})
            mlflow.log_metrics({"m1": random.random()})

            for step in range(10):
                mlflow.log_metrics({"m2": random.random()}, step=step)

            model = LinearRegression()
            registered_model_name = f"model_{i}" if i >= 40 else None
            mlflow.sklearn.log_model(
                model,
                "model",
                pip_requirements=["scikit-learn"],
                registered_model_name=registered_model_name,
            )

    for _ in range(50):
        with mlflow.start_run():
            mlflow.log_params({"p1": random.random(), "p3": random.random()})
            mlflow.log_metrics({"m1": random.random(), "m3": random.random()})


db_path = "mlflow.db"
if os.path.exists(db_path):
    os.remove(db_path)
mlflow.set_tracking_uri(f"sqlite:///{db_path}")

# Default experiment
log()

mlflow.set_experiment("a")
log()

mlflow.set_experiment("b")
log()

Does this PR change the documentation?

  • No. You can skip the rest of this section.
  • Yes. Make sure the changed pages / sections render correctly by following the steps below.
  1. Check the status of the ci/circleci: build_doc check. If it's successful, proceed to the
    next step, otherwise fix it.
  2. Click Details on the right to open the job page of CircleCI.
  3. Click the Artifacts tab.
  4. Click docs/build/html/index.html.
  5. Find the changed pages / sections and make sure they render correctly.

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), interfaces, languages, and integrations does this PR affect?

Components

  • area/artifacts: Artifact stores and artifact logging
  • area/build: Build and test infrastructure for MLflow
  • area/docs: MLflow documentation pages
  • area/examples: Example code
  • area/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registry
  • area/models: MLmodel format, model serialization/deserialization, flavors
  • area/projects: MLproject format, project running backends
  • area/scoring: MLflow Model server, model deployment tools, Spark UDFs
  • area/server-infra: MLflow Tracking server backend
  • area/tracking: Tracking Service, tracking client APIs, autologging

Interface

  • area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
  • area/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
  • area/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registry
  • area/windows: Windows support

Language

  • language/r: R APIs and clients
  • language/java: Java APIs and clients
  • language/new: Proposals for new client languages

Integrations

  • integrations/azure: Azure and Azure ML integrations
  • integrations/sagemaker: SageMaker integrations
  • integrations/databricks: Databricks integrations

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

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Comment on lines 1 to 7
mlflow.statsmodels
==================

.. automodule:: mlflow.statsmodels
:members:
:undoc-members:
:show-inheritance:
Copy link
Member Author

Choose a reason for hiding this comment

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

It appears line endings have been changed to LF from CRLF.

Copy link
Member Author

Choose a reason for hiding this comment

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

(mlflow-dev-env) @ mlflow [master] -- INSERT --
% git branch --show-current
master
                                                                                                                                                                                                                                                                                                                                                    
(mlflow-dev-env) @ mlflow [master] -- INSERT --
% file docs/source/python_api/mlflow.statsmodels.rst
docs/source/python_api/mlflow.statsmodels.rst: ASCII text, with CRLF line terminators
(mlflow-dev-env) @ mlflow [5hy2wakz] -- INSERT --
% git branch --show-current
5hy2wakz
                                                                                                                                                                                                                                                                                                                                                    
(mlflow-dev-env) @ mlflow [5hy2wakz] -- INSERT --
% file docs/source/python_api/mlflow.statsmodels.rst
docs/source/python_api/mlflow.statsmodels.rst: ASCII text

Copy link
Member Author

Choose a reason for hiding this comment

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

A list of files using CRLF

% git ls-files | xargs file | grep CRLF
docs/source/python_api/mlflow.statsmodels.rst:                                                                                          ASCII text, with CRLF line terminators
examples/statsmodels/MLproject:                                                                                                         ASCII text, with CRLF line terminators
examples/statsmodels/README.md:                                                                                                         ASCII text, with CRLF line terminators
examples/statsmodels/conda.yaml:                                                                                                        ASCII text, with CRLF line terminators
examples/statsmodels/train.py:                                                                                                          Python script text executable, ASCII text, with CRLF line terminators
mlflow/server/prometheus_exporter.py:                                                                                                   Python script text executable, ASCII text, with CRLF line terminators
tests/statsmodels/test_statsmodels_model_export.py:                                                                                     Python script text executable, ASCII text, with CRLF line terminators

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #5580 to convert CRLF to LF.

@github-actions github-actions bot added the rn/feature Mention under Features in Changelogs. label Apr 1, 2022
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Comment on lines 53 to 87
{this.state.listExperimentsExpanded ? (
<ExperimentListView
activeExperimentId={this.props.experimentId}
onClickListExperiments={this.onClickListExperiments}
/>
) : (
<i
onClick={this.onClickListExperiments}
title='Show experiment list'
style={styles.showExperimentListExpander}
className='expander fa fa-chevron-right login-icon'
/>
)}
<PageContainer>
{this.state.listExperimentsExpanded ? (
<ExperimentListView
activeExperimentId={this.props.experimentIds && this.props.experimentIds[0]}
onClickListExperiments={this.onClickListExperiments}
/>
) : (
<i
onClick={this.onClickListExperiments}
title='Show experiment list'
style={styles.showExperimentListExpander}
className='expander fa fa-chevron-right login-icon'
/>
)}
</PageContainer>
Copy link
Member Author

@harupy harupy Apr 4, 2022

Choose a reason for hiding this comment

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

Before: (the experiment list almost touches the top bar)

image

After:

image

cc @xanderwebs

@WeichenXu123
Copy link
Collaborator

I pull the branch and try to run locally, but hit an error:

(py38) weichen.xu@C02G36DSMD6R js % yarn start           
command not found: craco

I tried to run:
yarn add @craco/craco to install it, but failed again:

➤ YN0001: │ Error: While cloning /Users/weichen.xu/work/mlflow/mlflow/server/js/node_modules/humanize-ms/node_modules/ms -> /Users/weichen.xu/work/mlflow/mlflow/server/js/node_modules/send/node_modules/ms ENOENT: no such file or directory, scandir '/Users/weichen.xu/work/mlflow/mlflow/server/js/node_modules/humanize-ms/node_modules/ms'

@harupy
Copy link
Member Author

harupy commented Apr 4, 2022

@WeichenXu123 I think you forgot to run yarn install.

@WeichenXu123
Copy link
Collaborator

WeichenXu123 commented Apr 4, 2022

@WeichenXu123 I think you forgot to run yarn install.

No, I already run yarn install it does not address this. Then I tried solely run yarn add @craco/craco it failed like above.
yarn install also emit a error like:

➤ YN0001: │ Error: While cloning /Users/weichen.xu/work/mlflow/mlflow/server/js/node_modules/humanize-ms/node_modules/ms -> /Users/weichen.xu/work/mlflow/mlflow/server/js/node_modules/send/node_modules/ms ENOENT: no such file or directory, scandir '/Users/weichen.xu/work/mlflow/mlflow/server/js/node_modules/humanize-ms/node_modules/ms'

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
@harupy harupy requested a review from xanderwebs April 5, 2022 00:44
@harupy
Copy link
Member Author

harupy commented Apr 5, 2022

@xanderwebs Would you mind taking a brief look at this PR?

Copy link
Collaborator

@dbczumar dbczumar left a comment

Choose a reason for hiding this comment

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

@harupy Not sure if it's new with this PR, but the edit name / delete actions in the experiment sidebar are very close to the righthand side:

Screen Shot 2022-04-04 at 11 39 37 PM

Copy link
Collaborator

@dbczumar dbczumar left a comment

Choose a reason for hiding this comment

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

LGTM once #5577 (review) is addressed. Tested this out locally using a synthetic workflow from the PR description and confirmed that all components work properly. Thanks @harupy !

@harupy
Copy link
Member Author

harupy commented Apr 5, 2022

@harupy Not sure if it's new with this PR, but the edit name / delete actions in the experiment sidebar are very close to the righthand side:

Screen Shot 2022-04-04 at 11 39 37 PM

@dbczumar Thanks for the catch, let me check.

@harupy
Copy link
Member Author

harupy commented Apr 5, 2022

1.24.0:

image

1.23.0:

image

@dbczumar
Copy link
Collaborator

dbczumar commented Apr 5, 2022

1.24.0:

image

Can we increase the margin slightly?

@harupy
Copy link
Member Author

harupy commented Apr 5, 2022

Added a 10px (the same value as the edit button) right margin:

image

@dbczumar
Copy link
Collaborator

dbczumar commented Apr 5, 2022

Added a 10px (the same value as the edit button) right margin:

image

Thanks!

@harupy
Copy link
Member Author

harupy commented Apr 5, 2022

margin-right: 3px

image

margin-right: 5px

image

margin-right: 10px

image

@dbczumar Which one looks the best to you? 3px looks too narrow. 10px looks a bit too wide? 5px looks good to me.

@harupy
Copy link
Member Author

harupy commented Apr 5, 2022

The tag table on the run page also uses 10px:

image

Let's use 10px.

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
@@ -193,6 +193,7 @@ export class ExperimentListView extends Component {
onClick={this.handleDeleteExperiment}
data-experimentid={experiment_id}
data-experimentname={name}
style={{ marginRight: 10 }}
Copy link
Member Author

Choose a reason for hiding this comment

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

Change to fix #5577 (review)

Copy link
Collaborator

@WeichenXu123 WeichenXu123 left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +105 to +132
...(proxyTarget && {
devServer: {
hot: true,
https: true,
proxy: [
// Heads up src/setupProxy.js is indirectly referenced by CRA
// and also defines proxies.
{
context: function(pathname) {
return mayProxy(pathname);
},
pathRewrite: { '^/mfe/mlflow': '' },
target: proxyTarget,
secure: false,
changeOrigin: true,
ws: true,
xfwd: true,
onProxyRes: (proxyRes, req) => {
rewriteRedirect(proxyRes, req);
rewriteCookies(proxyRes);
},
},
],
host: 'localhost',
port: 3000,
open: false,
},
}),
Copy link
Member Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rn/feature Mention under Features in Changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants