fix: Added Git Actions using Command Pattern#4329
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a reversible workflow abstraction and a set of reversible Git actions to support submit_results-style automated git/GitHub operations with rollback, along with unit tests for both components.
Changes:
- Added
ReversibleWorkflow+ReversibleActionprotocol to execute steps and automatically roll back completed steps on failure. - Added concrete git/GitHub reversible actions (branching, copying results, committing, pushing, PR creation, branch restore).
- Added dependency group for PyGithub and unit tests covering workflow rollback and action do/undo behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
mteb/workflow/reversible_workflow.py |
New rollback-capable workflow orchestrator. |
mteb/workflow/git_actions.py |
New reversible actions wrapping git commands and PR creation. |
tests/test_workflow/test_reversible_workflow.py |
Tests for workflow success, rollback ordering, and undo-failure tolerance. |
tests/test_workflow/test_git_actions.py |
Tests for do/undo behavior of the new git actions (via monkeypatched subprocess / fake GH client). |
pyproject.toml |
Adds a github dependency group (PyGithub) and includes it in typing. |
Comments suppressed due to low confidence (1)
mteb/workflow/reversible_workflow.py:115
- If any
undo()call fails, the workflow still raisesRuntimeError(... 'All completed steps have been rolled back.'), which is inaccurate and can mislead callers into thinking the repo is clean. Track whether any undo failed and adjust the raised message (or include a list of failed undo step names) to reflect that rollback was only attempted / partially successful.
f"Undo for {rollback_name} also failed: {undo_error}. "
f"Manual cleanup may be required."
)
logger.error("Rollback completed. Raising exception...")
raise RuntimeError(
f"Workflow failed at {step_name}. All completed steps have been rolled back."
) from e
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@KennethEnevoldsen I need to add both git_actions and reversible_workflow in the same PR, but its going above 800 lines, as tests are also there. Would you be fine with that? |
|
@KennethEnevoldsen Could you review this one? |
KennethEnevoldsen
left a comment
There was a problem hiding this comment.
I think in general we want don't want to try to catch too many things doing the undo (if we start failing there we should just raise it fully)
|
@KennethEnevoldsen Can you once check on how exactly I should remove the |
|
Converted to draft. So, that it will not get stale and closed. |
|
@ayush1298 I answered you comment above |
|
@ayush1298 can you fix tests? |
Co-authored-by: Copilot <copilot@github.com>
Done |
|
@KennethEnevoldsen, can you check it again? Also, I have left a comment above regarding setting of config |
KennethEnevoldsen
left a comment
There was a problem hiding this comment.
@KennethEnevoldsen I have updated tests and removed monkeypatch. Regarding username and mail, those are need to be configured so that we can create commits during tests. Either we have to do it like this, or take it from environment variables. So, what will be a better way here?
I think the solution is to detect if it is set and only set it if it is not. Besides that I think the PR looks good. Sorry for the slow review, wanted to find the time to properly review it.
Made these changes. Totally agree that it's long PR, so need time. Also, I kept it open in meanwhile for long time after feedback |
|
Dont know why test leaderboard Dockerfile CI is getting timed out. |
|
It's failing all the time |
Did we tried to fix it? |
b2dfda9
into
embeddings-benchmark:main
This is 2nd PR as part of dividing #4239 into 3 parts.
In this PR, mainly 2 things are added.
ReversibleWorkflowusing command pattern and Git do and undo Actions corresponding to each git command. This PR will act as a base forsubmit_resultsfeature that will be added in #4239. Details are as follows:ReversibleWorkflow Pattern (added under
mteb/workflow/reversible_workflow.py)Purpose: Execute git operations with automatic rollback on failure
Git Actions (added under
mteb/workflow/git_actions.py)Concrete reversible actions for git operations:
CreateBranchAction[Create branch → undo: delete branch + restore original]CopyResultsAction[Copy files → undo: delete copied files]CommitAction[Create commit → undo: reset to previous commit]PushToForkAction[Push to fork → undo: force-push to revert]CreatePRAction[Create PR → undo: close PR]RestoreOriginalBranchAction[Restore to original branch → undo: return to previous]Also, tests are added under folder
test_workflowcorresponding to both files,reversible_workflowandgit_actions