Add a guard job to prevent accidental auto-merging of PRs when cross-version tests fail

[TomeHirata]

🛠 DevTools 🛠

Install mlflow from this PR

pip install git+https://github.com/mlflow/mlflow.git@refs/pull/10210/merge

Checkout with GitHub CLI

gh pr checkout 10210

Related Issues/PRs

Resolve #10069

What changes are proposed in this pull request?

Cross-version tests can't be marked as required because they get triggered conditionally.

How is this PR tested?

• Existing unit/integration tests

• New unit/integration tests

• Manual tests

• Story 1: When a PR has not completed all checks, this workflow skips the PR

  • https://github.com/TomeHirata/mlflow_10069/actions/runs/6684075777/job/18160951532

• Story 2: When a PR has completed all checks, this workflow merges the PR

  • Create README.md TomeHirata/mlflow_10069#1

Does this PR require documentation update?

• No. You can skip the rest of this section.
• Yes. I've updated:
  • Examples
  • API references
  • Instructions

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.

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/gateway: AI Gateway service, Gateway client APIs, third-party Gateway integrations
• area/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registry
• area/models: MLmodel format, model serialization/deserialization, flavors
• area/recipes: Recipes, Recipe APIs, Recipe configs, Recipe Templates
• 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/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/breaking-change - The PR will be mentioned in the "Breaking Changes" 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

[github-actions]

Documentation preview for a1cdac3 will be available here when this CircleCI job completes successfully.

More info

• Ignore this comment if this PR does not change the documentation.
• It takes a few minutes for the preview to be available.
• The preview is updated when a new commit is pushed to this PR.
• This comment was created by https://github.com/mlflow/mlflow/actions/runs/6871021327.

[TomeHirata]

Question: should we add an instruction about the auto-merge label to https://github.com/mlflow/mlflow/blob/master/CONTRIBUTING.md?

[harupy]

@TomeHirata I found a better and simpler solution to solve the original problem. Can we close this and revisit later in the future if necessary?

--- EDIT ---

The solution may not work. Let me think.

[harupy]

Question: should we add an instruction about the auto-merge label to https://github.com/mlflow/mlflow/blob/master/CONTRIBUTING.md?

Yes, we should, but we add it later as a follow-up after we confirm everything works as expected.

[TomeHirata]

@harupy I saw your PR was correctly merged after approving it. Please let me know if any further tests/modifications are needed. 🙏

[harupy]

@TomeHirata I found a few more issues in the current implementation.

1. It's possible that a single commit ref have multiple check runs for a single job. For example, two runs for the post-merge workflow (canceled one + succeded one).
2. CircleCI checks are not check runs. They are commit statues. We need to use a different GitHub API.

[TomeHirata]

That's a good point. I'll work on #2. Regarding the first issue of multiple checks, could you elaborate on why this will become an issue? Do we need to worry about the status of post-merge workflows before merging the PR?

[harupy]

could you elaborate on why this will become an issue? Do we need to worry about the status of post-merge workflows before merging the PR?

Yes, this isn't specific to the post-merge. The same thing can happen to other jobs. Imagine the following scenario:

1. Make a commit
2. Push the commit
3. Make another commit
4. Push it
5. Revert the second commit locally
6. Do a force-push

After this, the first commit two runs for each job. The first run could be canceled. If so, that would block automerge because cancled is not skipped or success.

[TomeHirata]

Thank you for the explanation, yes it is possible. Then, should we group the check runs by a unique check identifier (need to figure out) and check the status of the latest run?

[harupy]

Yes, that should work.

[TomeHirata]

@harupy I've updated the logic in 8bc132a. Could you take a look? Also, can I ask if there is any environment to verify the commit status check easily?

[harupy]

Thanks for the updates! Can we manually create a commit status using https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#create-a-commit-status?

[harupy]

@TomeHirata After some thought, I think adding a guard job is simpler and much safer than the current approach. The guard job does the following:

1. Always runs on pull request.
2. Is marked as required.
3. Wait for other jobs to succeed.

Example: #10368

I can push updates to this PR if this sounds good to you.

[TomeHirata]

That is similar to what I was thinking when I asked this question. I agree that having a synchronous job triggered per PR is better to work with. Is it correct that you plan to make the job required in the Github setting?

[harupy]

Is it correct that you plan to make the job required in the Github setting?

Yes, that's the plan.

[TomeHirata]

Sure, then this also sounds to me. Please feel free to push changes to the PR.

[harupy]

re-LGTM!

[harupy]

@TomeHirata Thanks for the patience! I underestimated the complexity of PR merge. Glad we settled on a safe and simple solution.

[harupy]

@TomeHirata Can we resume the work on #9939?

[TomeHirata]

@harupy Yes, what will be the next step of #9939? Should we start from the designing?
Also, would you mind reviewing #10266?

[harupy]

@TomeHirata Thanks for the reply!

Yes, what will be the next step of #9939? Should we start from the designing?

#9939 is actually like a design (for more details, please take a look at the attached PDF). We need to add unit tests and docs.

Design doc: Copy of [2023-11-18] One-Decision Doc_ Rate limits for OSS AI Gateway.pdf

Also, would you mind reviewing #10266?

Left some comments, thanks for working on this!

[TomeHirata]

Thank you for the reply, let me take a look at the doc and the current state of the PR.