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

queue remove: can remove successed,failed tasks msgs #7924

Merged

Conversation

karajan1001
Copy link
Contributor

@karajan1001 karajan1001 commented Jun 21, 2022

Followed from #7592

  1. Seperate remove method to a new file.
  2. Add queued, failed, processed flags to remove.
  3. Add new unit tests for queue status --queue/fail/success
  4. Implement methods to remove done tasks.
  5. add two new unit test for celery_queue.remove
  6. bump into dvc-task version 0.0.13

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

A demo for this PR.
asciicast

@karajan1001 karajan1001 requested a review from a team as a code owner June 21, 2022 12:02
@karajan1001 karajan1001 removed the request for review from a team June 21, 2022 12:02
@karajan1001 karajan1001 self-assigned this Jun 21, 2022
@karajan1001 karajan1001 requested a review from skshetry June 21, 2022 12:02
@karajan1001 karajan1001 marked this pull request as draft June 21, 2022 12:02
@karajan1001 karajan1001 requested review from pmrowla and removed request for skshetry June 21, 2022 12:02
@skshetry skshetry changed the title [WIP]Queue remove: Can remove successed,failed tasks msgs [WIP] queue remove: Can remove successed,failed tasks msgs Jun 21, 2022
@skshetry skshetry changed the title [WIP] queue remove: Can remove successed,failed tasks msgs [WIP] queue remove: can remove successed,failed tasks msgs Jun 21, 2022
@karajan1001 karajan1001 force-pushed the queue_remove_support_finished_exps branch 3 times, most recently from c97735f to 58db6d7 Compare June 28, 2022 04:03
@karajan1001
Copy link
Contributor Author

karajan1001 commented Jun 28, 2022

@pmrowla
The only problem remained is that the queued experiment I prepared will be executed wrongly by some celery worker in the background, making the tests really unstable. Any ideas about it?

@karajan1001 karajan1001 changed the title [WIP] queue remove: can remove successed,failed tasks msgs queue remove: can remove successed,failed tasks msgs Jun 28, 2022
@karajan1001 karajan1001 marked this pull request as ready for review June 28, 2022 04:36
@pmrowla
Copy link
Contributor

pmrowla commented Jun 28, 2022

@pmrowla The only problem remained is that the queued experiment I prepared will be executed wrongly by some celery worker in the background, making the tests really unstable. Any ideas about it?

@karajan1001 it's likely due to using the celery pytest worker inside multiple fixtures at once.

In general, we should try to move away from adding complex func tests in favor of unit testing whenever possible. In this case, for celery we do need a couple of func tests to make sure that execution and the UI work properly (in both failed and successful cases), but for testing specific backend or API calls (like remove/status) it's better to use unit tests + mocked celery behavior

@pmrowla
Copy link
Contributor

pmrowla commented Jun 29, 2022

One other thing to note here regarding commits, ideally we want to keep changes in separate commits where possible. In this PR it would be nice to have the separation with:

  • 1 commit for the dependency version bump. If there are specific changes required to make the new dep version work with DVC then those changes can also go into the version bump commit (like when a dependency has a backwards incompatible API change)
  • 1 commit for the queue remove related changes
  • 1 commit for the queue status related changes

Basically for small "review fix" type changes it makes sense to squash them into the larger commit, but for larger independent changes that affect different parts of the code base or different parts of the CLI/API, having separate commits makes it easier to keep track of what changed and when. So follow-up changes/fixes to remove or status can get squashed into each relevant commit before merging the PR, but ideally the final PR would not contain commits that mix changes to both remove and status.


This isn't really a requirement for this PR, but it's something to be aware of in the future

@karajan1001
Copy link
Contributor Author

One other thing to note here regarding commits, ideally we want to keep changes in separate commits where possible. In this PR it would be nice to have the separation with:

  • 1 commit for the dependency version bump. If there are specific changes required to make the new dep version work with DVC then those changes can also go into the version bump commit (like when a dependency has a backwards incompatible API change)
  • 1 commit for the queue remove related changes
  • 1 commit for the queue status related changes

After removing the functional test of queue status. It only contains queue remove related changes and the dependency bump. I can separate them later.

@karajan1001 karajan1001 force-pushed the queue_remove_support_finished_exps branch 2 times, most recently from 0ad89c5 to 3293471 Compare June 30, 2022 06:01
Followed from iterative#7592

1. Seperate remove method to a new file.
2. Add queued, failed, processed flags to remove.
3. Add new unit tests for queue status --queue/fail/success
4. Implement methods to remove done tasks.
5. add some new unit and functional test for celery_queue.remove
6. bump into dvc-task version 0.0.13
@karajan1001 karajan1001 force-pushed the queue_remove_support_finished_exps branch from 3293471 to 8efda22 Compare June 30, 2022 06:03
assert "failed to reproduce 'failed-copy-file'" in captured.out


def test_queue_remove_done(dvc, failed_tasks, success_tasks):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need still need this test? With unit tests for remove() we should not need any additional func tests here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can simplify them. We tested the remove method case by case as well as the cli test. But we don't have a place to test the whole process. This is why I added a functional test here, But I think we can simplify it, We don't need to test all of the cases, but only one of them.

@karajan1001 karajan1001 force-pushed the queue_remove_support_finished_exps branch from 97db11f to 000133a Compare July 4, 2022 09:58
@pmrowla pmrowla merged this pull request into iterative:dvc-task-dev Jul 5, 2022
@karajan1001 karajan1001 deleted the queue_remove_support_finished_exps branch July 5, 2022 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: experiments Related to dvc exp
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants