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

[Bugfix] Alter_column on metrics table only called for mysql database engine #4880

Merged
merged 9 commits into from
Oct 13, 2021

Conversation

marijncv
Copy link
Contributor

@marijncv marijncv commented Oct 8, 2021

What changes are proposed in this pull request?

The migration needed to (re)set the default value for is_nan in the metrics table is now only run for the mysql database engine. Closes #4343

How is this patch tested?

Running the database initialization tests in the github workflow. I've added a test for the mssql engine.

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.

Fix for bug that occurred in initialization/migration of the Tracking Server backend using a mssql engine.

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: Marijn Valk <marijncv@hotmail.com>
Signed-off-by: Marijn Valk <marijncv@hotmail.com>
@github-actions github-actions bot added area/server-infra MLflow Tracking server backend area/sqlalchemy Use of SQL alchemy in tracking service or model registry rn/bug-fix Mention under Bug Fixes in Changelogs. labels Oct 8, 2021
Signed-off-by: Marijn Valk <marijncv@hotmail.com>
Signed-off-by: Marijn Valk <marijncv@hotmail.com>
@dbczumar dbczumar requested a review from harupy October 8, 2021 23:54
@github-actions github-actions bot added the area/build Build and test infrastructure for MLflow label Oct 10, 2021
@harupy
Copy link
Member

harupy commented Oct 12, 2021

@marijncv Thanks for the PR! I'll review soon!

tests/db/init-mssql-db.sql Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
#!/bin/bash
sleep 60 # wait for SQL Server to startup
Copy link
Member

@harupy harupy Oct 12, 2021

Choose a reason for hiding this comment

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

Is there any command that can check the SQL server is ready? I'm wondering if we can do something like below to avoid always sleeping for 60 seconds:

for BACKOFF in 0 1 2 4 8 16 32 64; do
    sleep $BACKOFF
    if <check_sql_server_is_ready>; then
        exit 0
    fi
done
exit 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @harupy for this great suggestion! I've updated the script to use this retry mechanism

Signed-off-by: Marijn Valk <marijncv@hotmail.com>
@GlamTam
Copy link

GlamTam commented Oct 12, 2021

tested it in our env and works great!

tests/db/init-mssql-db.sql Outdated Show resolved Hide resolved
tests/db/Dockerfile Outdated Show resolved Hide resolved
tests/db/init-mssql-db.sh Outdated Show resolved Hide resolved
@harupy
Copy link
Member

harupy commented Oct 12, 2021

@marijncv Thanks for the PR and creating tests for mssql! Left some comments.

Signed-off-by: Marijn Valk <marijncv@hotmail.com>
@marijncv
Copy link
Contributor Author

Thanks for the comments! I've tried to address them in the latest commit

Copy link
Member

@harupy harupy left a comment

Choose a reason for hiding this comment

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

LGTM!

@harupy harupy merged commit 40b62db into mlflow:master Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build and test infrastructure for MLflow area/server-infra MLflow Tracking server backend area/sqlalchemy Use of SQL alchemy in tracking service or model registry rn/bug-fix Mention under Bug Fixes in Changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Alembic migration for metrics table uses incorrect server default
3 participants