Skip to content

Backport Mlflow AutoPilot E2E fix to 1.13 - #30221

Merged
pmbrull merged 1 commit into
1.13from
cherry-30191-1.13
Jul 20, 2026
Merged

Backport Mlflow AutoPilot E2E fix to 1.13#30221
pmbrull merged 1 commit into
1.13from
cherry-30191-1.13

Conversation

@IceS2

@IceS2 IceS2 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Backports the Playwright half of #30191 to 1.13. 1.13 carries the same mlflow-skinny>=3.13.0,<3.15 bump, so the AutoPilot E2E fails here the same way.

What

MlFlowIngestionClass filled placeholder connection strings that MLflow resolved to a local file store, so the agent walked an empty registry and the test passed. MLflow 3.13 put the file store into maintenance mode and now raises instead:

MlflowException: The filesystem tracking backend (e.g., './mlruns') is in maintenance mode
and will not receive further updates. ... set `MLFLOW_ALLOW_FILE_STORE=true` to opt out.

The agent throws on connect, the AutoPilot workflow never completes successfully, and the run-completed banner never renders. Verified against the pinned version:

mlflow-skinny search_registered_models() with a placeholder URI
3.11.1 (old ceiling) OK — file store accepted
3.13.0 (new floor) raises MlflowException

The fields now hold sqlite DSNs. SQLite is a supported backend that bootstraps its own schema, so it keeps the "empty registry, agent succeeds" semantics the test already relied on — as a deliberate choice rather than a side effect of MLflow's string parsing. Both fields are plain type: string in the schema, so no schema or UI change is needed.

Scope

Playwright only, and intentionally byte-identical to the change already merged on main so the branches do not diverge.

Greptile flagged that the fixed /tmp paths are shared across services in the ingestion container (possible lock contention on concurrent first-time schema creation). That is a fair point but applies equally to main, which already merged these paths — so it should be hardened there first and backported, not introduced here as a divergence.

The connector-side version-selection fix from #30191 and its regression test are not included here, by request — 1.13 still carries that bug (a registered model is dropped after its description/tags/aliases are edited). The test change is the regression guard for that fix, so both were dropped together; keeping the test without the code fix would leave a failing integration test.

Greptile Summary

This PR updates the MLflow Playwright setup for MLflow 3.13 compatibility. The main changes are:

  • Replaces placeholder tracking and registry URIs with SQLite DSNs.
  • Preserves the empty-registry behavior without requiring a live MLflow service.
  • Limits the backport to the Playwright fixture.

Confidence Score: 5/5

No additional blocking issue qualifies for this follow-up review.

  • No distinct unresolved failure remains after excluding the already reported SQLite isolation issue.

Important Files Changed

Filename Overview
openmetadata-ui/src/main/resources/ui/playwright/support/entity/ingestion/MlFlowIngestionClass.ts Updates the MLflow Playwright connection fields to use SQLite tracking and registry backends supported by MLflow 3.13.

Reviews (3): Last reviewed commit: "fix(ui): unbreak the Mlflow AutoPilot E2..." | Re-trigger Greptile

Context used:

  • Context used - CLAUDE.md (source)
  • Context used - openmetadata-ui-core-components/CLAUDE.md (source)

@IceS2
IceS2 requested review from a team as code owners July 20, 2026 08:00
@github-actions

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

  • No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically.

Maintainers can bypass this check by adding the skip-pr-checks label.

@github-actions github-actions Bot added Ingestion safe to test Add this label to run secure Github workflows on PRs labels Jul 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

The Python checkstyle failed.

Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

@sonarqubecloud

Copy link
Copy Markdown

@IceS2
IceS2 force-pushed the cherry-30191-1.13 branch from dd3eb74 to 1ce78a4 Compare July 20, 2026 09:02
@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

🟡 Playwright Results — all passed (11 flaky)

✅ 3968 passed · ❌ 0 failed · 🟡 11 flaky · ⏭️ 30 skipped

Shard Passed Failed Flaky Skipped
✅ Shard 1 291 0 0 4
✅ Shard 2 0 0 0 0
🟡 Shard 3 735 0 2 7
🟡 Shard 4 780 0 3 2
🟡 Shard 5 754 0 3 9
✅ Shard 6 693 0 0 0
🟡 Shard 7 715 0 3 8
🟡 11 flaky test(s) (passed on retry)
  • Features/BulkEditEntity.spec.ts › Database Schema (shard 3, 1 retry)
  • Features/BulkImport.spec.ts › Database service (shard 3, 1 retry)
  • Features/Permissions/GlossaryPermissions.spec.ts › Team-based permissions work correctly (shard 4, 1 retry)
  • Flow/ExploreAggregationCountsMatching.spec.ts › should verify left panel counts and tab search results for normal search (shard 4, 2 retries)
  • Flow/PersonaFlow.spec.ts › Set default persona for team should work properly (shard 4, 1 retry)
  • Pages/CustomProperties.spec.ts › Enum (shard 5, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Spreadsheet (shard 5, 1 retry)
  • Pages/Domains.spec.ts › Rename domain with subdomains attached verifies subdomain accessibility (shard 5, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify lineage schema filter selection (shard 7, 1 retry)
  • Pages/TestSuite.spec.ts › Logical TestSuite (shard 7, 1 retry)
  • Pages/Users.spec.ts › Permissions for table details page for Data Consumer (shard 7, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@IceS2 IceS2 changed the title Backport #30191 to 1.13: Repair Mlflow version selection and AutoPilot E2E Backport Mlflow AutoPilot E2E fix to 1.13 Jul 20, 2026
Point the Playwright Mlflow connection at sqlite backends. The placeholder
strings resolved to MLflow's file store, which 3.13+ rejects, so the AutoPilot
agent failed to connect and the run-completed banner never rendered.

Backport of the Playwright change in #30191.
@IceS2
IceS2 force-pushed the cherry-30191-1.13 branch from 1ce78a4 to 05a1423 Compare July 20, 2026 09:07
// MLflow 3.13+ rejects the filesystem backend, which is what a non-URI
// string resolves to. SQLite is a supported backend and bootstraps itself,
// yielding an empty registry the agent can walk without a live MLflow.
await page.fill('#root\\/trackingUri', 'sqlite:////tmp/mlflow_tracking.db');

@gitar-bot gitar-bot Bot Jul 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Bug: SQLite DB paths are shared, not scoped per service instance

The PR description states the SQLite paths are "scoped per service instance so concurrent runs sharing the ingestion container cannot contend on one file or inherit a half-created schema from an earlier attempt," but the committed code uses fixed paths for the tracking and registry databases. Any parallel Playwright workers/services or a retry after a partial failure will contend on the same files or inherit a half-created schema, reintroducing exactly the flakiness the change claims to prevent. Since this.serviceName (which embeds a uuid()) is available on the base class, derive the DB filenames from it, e.g., sqlite:////mlflow_tracking_${this.serviceName}.db.

Scope both SQLite backends per service instance using the unique serviceName.:

await page.fill(
  '#root\/trackingUri',
  `sqlite:////tmp/mlflow_tracking_${this.serviceName}.db`
);
await checkServiceFieldSectionHighlighting(page, 'trackingUri');
await page.fill(
  '#root\/registryUri',
  `sqlite:////tmp/mlflow_registry_${this.serviceName}.db`
);
await checkServiceFieldSectionHighlighting(page, 'registryUri');

Was this helpful? React with 👍 / 👎

@github-actions

Copy link
Copy Markdown
Contributor

The Python checkstyle failed.

Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

@pmbrull
pmbrull merged commit aaa86c8 into 1.13 Jul 20, 2026
49 of 54 checks passed
@pmbrull
pmbrull deleted the cherry-30191-1.13 branch July 20, 2026 09:23
@gitar-bot

gitar-bot Bot commented Jul 20, 2026

Copy link
Copy Markdown
Code Review ⚠️ Changes requested 0 resolved / 1 findings

Backports MLflow AutoPilot E2E test fixes to 1.13, but the implementation uses fixed paths instead of service-scoped SQLite database paths, risking collision between concurrent test runs.

⚠️ Bug: SQLite DB paths are shared, not scoped per service instance

📄 openmetadata-ui/src/main/resources/ui/playwright/support/entity/ingestion/MlFlowIngestionClass.ts:61 📄 openmetadata-ui/src/main/resources/ui/playwright/support/entity/ingestion/MlFlowIngestionClass.ts:63

The PR description states the SQLite paths are "scoped per service instance so concurrent runs sharing the ingestion container cannot contend on one file or inherit a half-created schema from an earlier attempt," but the committed code uses fixed paths for the tracking and registry databases. Any parallel Playwright workers/services or a retry after a partial failure will contend on the same files or inherit a half-created schema, reintroducing exactly the flakiness the change claims to prevent. Since this.serviceName (which embeds a uuid()) is available on the base class, derive the DB filenames from it, e.g., sqlite:////mlflow_tracking_${this.serviceName}.db.

Scope both SQLite backends per service instance using the unique serviceName.
await page.fill(
  '#root\/trackingUri',
  `sqlite:////tmp/mlflow_tracking_${this.serviceName}.db`
);
await checkServiceFieldSectionHighlighting(page, 'trackingUri');
await page.fill(
  '#root\/registryUri',
  `sqlite:////tmp/mlflow_registry_${this.serviceName}.db`
);
await checkServiceFieldSectionHighlighting(page, 'registryUri');
🤖 Prompt for agents
Code Review: Backports MLflow AutoPilot E2E test fixes to 1.13, but the implementation uses fixed paths instead of service-scoped SQLite database paths, risking collision between concurrent test runs.

1. ⚠️ Bug: SQLite DB paths are shared, not scoped per service instance
   Files: openmetadata-ui/src/main/resources/ui/playwright/support/entity/ingestion/MlFlowIngestionClass.ts:61, openmetadata-ui/src/main/resources/ui/playwright/support/entity/ingestion/MlFlowIngestionClass.ts:63

   The PR description states the SQLite paths are "scoped per service instance so concurrent runs sharing the ingestion container cannot contend on one file or inherit a half-created schema from an earlier attempt," but the committed code uses fixed paths for the tracking and registry databases. Any parallel Playwright workers/services or a retry after a partial failure will contend on the same files or inherit a half-created schema, reintroducing exactly the flakiness the change claims to prevent. Since `this.serviceName` (which embeds a `uuid()`) is available on the base class, derive the DB filenames from it, e.g., `sqlite:////mlflow_tracking_${this.serviceName}.db`.

   Fix (Scope both SQLite backends per service instance using the unique serviceName.):
   await page.fill(
     '#root\/trackingUri',
     `sqlite:////tmp/mlflow_tracking_${this.serviceName}.db`
   );
   await checkServiceFieldSectionHighlighting(page, 'trackingUri');
   await page.fill(
     '#root\/registryUri',
     `sqlite:////tmp/mlflow_registry_${this.serviceName}.db`
   );
   await checkServiceFieldSectionHighlighting(page, 'registryUri');

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ingestion safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants