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.

Sign up for GitHub

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

Jump to bottom

[Workflow] workflow.delete #19178

Merged
merged 12 commits into from
Oct 8, 2021
Merged

[Workflow] workflow.delete #19178

merged 12 commits into from
Oct 8, 2021

Conversation

wuisawesome
Copy link
Contributor

@wuisawesome wuisawesome commented Oct 7, 2021 •
edited by fishbone
Loading

Why are these changes needed?

This PR implements workflow.delete which allows users to delete the information in storage related to a workflow. (This assumes the workflow isn't currently running).

Related issue number

Closes #18848

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

This change is Reviewable

@wuisawesome
Copy link
Contributor Author

cc @lchu-ibm we'll probably want to add workflow.delete(namespace="...") once you finish implementing namespaces.

fishbone
fishbone reviewed Oct 7, 2021
View reviewed changes
python/ray/workflow/api.py Outdated Show resolved Hide resolved
fishbone
fishbone reviewed Oct 7, 2021
View reviewed changes
python/ray/workflow/tests/test_storage.py Outdated
workflow.resume("finishes")

# Deleting is idempotent.
workflow.delete(workflow_id="finishes")
Copy link
Contributor

Choose a reason for hiding this comment

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

We should throw JobNotFound error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't exist yet right?

Also nit: let's call it WorkflowNotFoundError ?

fishbone
fishbone reviewed Oct 7, 2021
View reviewed changes
python/ray/workflow/api.py Outdated Show resolved Hide resolved
fishbone
fishbone reviewed Oct 7, 2021
View reviewed changes
python/ray/workflow/api.py Outdated Show resolved Hide resolved
fishbone
fishbone reviewed Oct 7, 2021
View reviewed changes
python/ray/workflow/api.py Outdated Show resolved Hide resolved
wuisawesome
wuisawesome commented Oct 8, 2021
View reviewed changes
python/ray/workflow/execution.py
@@ -125,7 +125,9 @@ def get_status(workflow_id: str) -> Optional[WorkflowStatus]:
store = workflow_storage.get_workflow_storage(workflow_id)
meta = store.load_workflow_meta()
if meta is None:
raise ValueError(f"No such workflow_id {workflow_id}")
raise WorkflowNotFoundError(workflow_id)
if meta.status == WorkflowStatus.RUNNING:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like a necessary fix, tested in test_recovery

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

fishbone
fishbone requested changes Oct 8, 2021
View reviewed changes
Copy link
Contributor

@fishbone fishbone left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 7 files at r1, 3 of 5 files at r2, 2 of 6 files at r4, 1 of 1 files at r5.
Reviewable status: 7 of 10 files reviewed, 10 unresolved discussions (waiting on @iycheng and @wuisawesome)

python/ray/workflow/api.py, line 368 at r5 (raw file):

    print("STATUS:", status)

delete print

python/ray/workflow/workflow_storage.py, line 451 at r3 (raw file):

        scan = []
        scan_future = self._storage.scan_prefix(prefix)

should we just check exists first and then delete it?

fishbone
fishbone requested changes Oct 8, 2021
View reviewed changes
Copy link
Contributor

@fishbone fishbone left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 10 files reviewed, 11 unresolved discussions (waiting on @iycheng and @wuisawesome)

python/ray/workflow/tests/test_recovery.py, line 171 at r5 (raw file):

        simple.step("x").run(workflow_id=workflow_id)

    assert workflow.get_status(

good catch

fishbone
fishbone reviewed Oct 8, 2021
View reviewed changes
Copy link
Contributor

@fishbone fishbone left a comment

Choose a reason for hiding this comment

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

Overall it looks good. I tried reviewable and it looks awful :( Please ignore any wired comments.

python/ray/workflow/execution.py
@@ -125,7 +125,9 @@ def get_status(workflow_id: str) -> Optional[WorkflowStatus]:
store = workflow_storage.get_workflow_storage(workflow_id)
meta = store.load_workflow_meta()
if meta is None:
raise ValueError(f"No such workflow_id {workflow_id}")
raise WorkflowNotFoundError(workflow_id)
if meta.status == WorkflowStatus.RUNNING:
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

python/ray/workflow/api.py Outdated Show resolved Hide resolved
python/ray/workflow/workflow_storage.py
Comment on lines +450 to +452
scan = []
scan_future = self._storage.scan_prefix(prefix)
delete_future = self._storage.delete_prefix(prefix)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we just check exists and delete it? (https://sourcegraph.com/github.com/ray-project/ray/-/blob/python/ray/workflow/workflow_storage.py?L375) ?

fishbone
fishbone approved these changes Oct 8, 2021
View reviewed changes
Copy link
Contributor

@fishbone fishbone left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Synced offline. We need scan_prefix a generator kind of thing for efficiency which can be done later. (#19234)

@wuisawesome
Copy link
Contributor Author

workflow tests passed and docs look fine in the preview. merging.

@wuisawesome wuisawesome merged commit 7ea512f into ray-project:master Oct 8, 2021
ericl added a commit that referenced this pull request Oct 9, 2021
@ericl
Revert "[Workflow] workflow.delete (#19178)"
1716a3b 
This reverts commit 7ea512f.
wuisawesome pushed a commit that referenced this pull request Oct 9, 2021
@ericl
Revert "[Workflow] workflow.delete (#19178)" (#19247)
b593175 
This reverts commit 7ea512f.
wuisawesome added a commit that referenced this pull request Oct 9, 2021
@wuisawesome
Revert "Revert "[Workflow] workflow.delete (#19178)" (#19247)"
a8e1379 
This reverts commit b593175.
wuisawesome pushed a commit that referenced this pull request Oct 19, 2021
Revert "[Hotfix] Revert "[Workflow] workflow.delete"" (#19248)
a819e41 
* Revert "Revert "[Workflow] workflow.delete (#19178)" (#19247)"

This reverts commit b593175.

* fix

* .

* .

* .

* Revert "."

This reverts commit 423b9b8.

* .

* .

* done?

* 4real

Co-authored-by: Alex <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fishbone fishbone fishbone approved these changes

Assignees

@fishbone fishbone

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature][workflow] Delete virtual actor after use
2 participants
@wuisawesome @fishbone