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

DB: Add environment variable option to skip DB table creationˆ #2245

Merged
merged 3 commits into from
Jan 4, 2024

Conversation

lkaybob
Copy link
Contributor

@lkaybob lkaybob commented Dec 3, 2023

What this PR does / why we need it:

Currently, DB Manager automatically tries to create observation_logs table on startup using CREATE TABLE IF NOT EXISTS clause. However, this part does not work when DB user does not have enough privilege to create table if DB schema creation is handled by manual admin process.

To address this, we have hard-forked the repository and built our own image after commenting out table creation part so far. Since I guess there might be similar need to skip table creation from other than us, I've added an environment variable option to skip table creation along with simple logic to validate observation_logs table structure with SELECT statement.

(ml-metadata allows this with option in protobuf text format.)

Which issue(s) this PR fixes

None. I've create pull request directly.

Checklist:

  • Docs included if any changes are user facing

(Need to update list of Environment Variables for Katib Components on Katib DB Manager. I will create additional pull request to address the change on website.)

FYI. I've created an issue for Pipelines(kubeflow/pipelines#10265) where similar improvement could be possible

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thank you for creating this @lkaybob!
I agree, we should give user capability to skip DB creation.
I left a few comments.
/assign @tenzen-y @johnugeorge

pkg/db/v1beta1/common/const.go Outdated Show resolved Hide resolved
pkg/util/v1beta1/env/env.go Outdated Show resolved Hide resolved
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

@lkaybob Thank you for this contribution :)
Totally, LGTM: I left a few comments.

pkg/db/v1beta1/mysql/init.go Show resolved Hide resolved
pkg/db/v1beta1/mysql/init.go Show resolved Hide resolved
pkg/db/v1beta1/postgres/init.go Outdated Show resolved Hide resolved
pkg/db/v1beta1/postgres/init.go Show resolved Hide resolved
Copy link
Contributor Author

@lkaybob lkaybob left a comment

Choose a reason for hiding this comment

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

Thanks for the review! @andreyvelich @tenzen-y
I've applied suggestions you left with additional changes to make consistent.

pkg/db/v1beta1/common/const.go Outdated Show resolved Hide resolved
pkg/util/v1beta1/env/env.go Outdated Show resolved Hide resolved
pkg/db/v1beta1/mysql/init.go Show resolved Hide resolved
pkg/db/v1beta1/postgres/init.go Show resolved Hide resolved
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

It looks great to me.
@kubeflow/wg-automl-leads Can you approve workflows?

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Dec 22, 2023
@tenzen-y
Copy link
Member

tenzen-y commented Dec 22, 2023

/lgtm cancel
(We need to wait for succeeded workflows)

@google-oss-prow google-oss-prow bot removed the lgtm label Dec 22, 2023
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkaybob, tenzen-y

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@andreyvelich
Copy link
Member

Thank you for the updates @lkaybob!
Please can you create an issue or website PR to track the documentation updates that are required for Katib

@tenzen-y
Copy link
Member

tenzen-y commented Jan 3, 2024

@kubeflow/wg-automl-leads Could you restart CI jobs, Go Test / Unit Test (1.26.1) and Go Test / Unit Test (1.27.1)?

@andreyvelich
Copy link
Member

andreyvelich commented Jan 3, 2024

It's strange why trial_controller_test constantly fails.
@lkaybob Please can you try to rebase your PR ?

@lkaybob
Copy link
Contributor Author

lkaybob commented Jan 3, 2024

@andreyvelich I've just rebased my PR, and I will create an issue at kubeflow/website repository to address this change so that I can work on it later.

@tenzen-y
Copy link
Member

tenzen-y commented Jan 4, 2024

This is blocked by #2251

@andreyvelich
Copy link
Member

@lkaybob Please can you rebase one more time ?
It should fix the Python test.

@lkaybob
Copy link
Contributor Author

lkaybob commented Jan 4, 2024

@andreyvelich I've just rebased the branch. Let's see if the test passes :)

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thank you for this @lkaybob!
/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Jan 4, 2024
@google-oss-prow google-oss-prow bot merged commit d92c168 into kubeflow:master Jan 4, 2024
59 checks passed
@lkaybob lkaybob deleted the skip-db-migration branch January 6, 2024 07:56
@andreyvelich andreyvelich mentioned this pull request Jan 24, 2024
10 tasks
@andreyvelich andreyvelich added this to the Katib v0.17.0 Release milestone Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants