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

TaskCancellation #7669

Merged
merged 50 commits into from
Apr 25, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
169c540
Smol comment
ijrsvt Mar 19, 2020
682c5b5
Merge branch 'master' into TaskCancellation
ijrsvt Mar 25, 2020
2d020ba
WIP, not passing ray.init
ijrsvt Mar 25, 2020
1958e05
Fixed small problem
ijrsvt Mar 25, 2020
4fdeb5a
wip
ijrsvt Mar 31, 2020
40b2bb5
Pseudo interrupt things
ijrsvt Mar 31, 2020
d1295c3
Basic prototype operational
ijrsvt Mar 31, 2020
269a3b1
Merge branch 'master' of github.com:ijrsvt/ray into TaskCancellation
ijrsvt Apr 1, 2020
028d9f7
correct proc title
ijrsvt Apr 2, 2020
a4b58e5
Mostly done
ijrsvt Apr 7, 2020
33ad6a1
Cleanup
ijrsvt Apr 7, 2020
4f7eec7
cleaner raylet error
ijrsvt Apr 7, 2020
cc3ca28
Cleaning up a few loose ends
ijrsvt Apr 7, 2020
bd47066
Fixing Race Conds
ijrsvt Apr 7, 2020
c0b5ab4
Prelim testing
ijrsvt Apr 7, 2020
58c8bed
Fixing comments and adding second_check for kill
ijrsvt Apr 8, 2020
bae435f
Working_new_impl
ijrsvt Apr 9, 2020
9ab039d
demo_ready
ijrsvt Apr 9, 2020
d85496d
Fixing my english
ijrsvt Apr 10, 2020
d0ba816
Merge branch 'master' into TaskCancellation
ijrsvt Apr 10, 2020
652a0fe
Fixing a few problems
ijrsvt Apr 10, 2020
daac610
Small problems
ijrsvt Apr 10, 2020
b050b28
Cleaning up
ijrsvt Apr 10, 2020
b0457a3
Response to changes
ijrsvt Apr 15, 2020
18b3dbc
Fixing error passing
ijrsvt Apr 15, 2020
b813faf
Merge branch 'master' into TaskCancellation
ijrsvt Apr 15, 2020
112d7d8
Merged to master
ijrsvt Apr 15, 2020
ff8bbd3
fixing lock
ijrsvt Apr 15, 2020
af35898
Cleaning up print statements
ijrsvt Apr 15, 2020
b015c51
Format
ijrsvt Apr 15, 2020
616f487
Fixing Unit test build failure
ijrsvt Apr 16, 2020
2361273
mock_worker fix
ijrsvt Apr 16, 2020
9dba915
java_fix
ijrsvt Apr 16, 2020
9a43056
Canel
ijrsvt Apr 16, 2020
68a6458
Switching to Cancel
ijrsvt Apr 17, 2020
46545e1
Responding to Review
ijrsvt Apr 21, 2020
7308225
FixFormatting
ijrsvt Apr 21, 2020
1f95492
Merge branch 'master' into TaskCancellation
ijrsvt Apr 21, 2020
9a0d120
Lease cancellation
ijrsvt Apr 22, 2020
82a6248
FInal comments?
ijrsvt Apr 22, 2020
3270f92
Moving exist check to CoreWorker
ijrsvt Apr 23, 2020
794f146
Fix Actor Transport Test
ijrsvt Apr 23, 2020
e43ea33
Fixing task manager test
ijrsvt Apr 23, 2020
9beea80
chaning clock repr
ijrsvt Apr 23, 2020
2a789f5
Fix build
ijrsvt Apr 24, 2020
8c75a83
fix white space
ijrsvt Apr 24, 2020
8f7bdfe
lint fix
ijrsvt Apr 24, 2020
6f1ef56
Updating to medium size
ijrsvt Apr 24, 2020
f7fb69f
Fixing Java test compilation issue
ijrsvt Apr 25, 2020
e8cd360
lengthen bad timeouts
ijrsvt Apr 25, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Cleaning up
  • Loading branch information
ijrsvt committed Apr 10, 2020
commit b050b28e8b5393f77bc49093cde8f39c65a5ebea
3 changes: 1 addition & 2 deletions python/ray/_raylet.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -559,14 +559,13 @@ cdef void async_plasma_callback(CObjectID object_id,
event_handler._loop.call_soon_threadsafe(
event_handler._complete_future, obj_id)

cdef c_bool kill_main_task() nogil:
cdef void kill_main_task() nogil:
with gil:
# This prevents this interrupt from being called to early. It may still
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused about what "too early" means and why we need this check of the getproctitle. If this is holding the CoreWorker::mutex_, don't we know for sure that the Python process is currently executing the right task?

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 comment is mostly incorrect at this point.

# be called later, but will not cancel another task because this is
# called with a C++ mutex lock.
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the interrupt gets delivered outside of the task execution handler's try-except block? Could that potentially crash the whole Python process?

Also, I might be missing something, but I don't think this reasoning is correct about it being impossible to cancel another task. Looking at the cython source code, it seems that interrupt_main just calls PyErr_SetInterrupt(), which will only trigger the signal the next time PyErr_CheckSignals() is called. I don't think we can guarantee that the thread has not started another task by the time the signal is checked by the main thread.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We might be able to address this by always calling PyErr_CheckSignals() in some known location (e.g., just before returning from cython to c++), but it'll take some thought. The python interpreter doesn't give a guarantee on when this will be raised unless you call it manually.

if setproctitle.getproctitle() != "ray::IDLE":
_thread.interrupt_main()
return True


cdef CRayStatus check_signals() nogil:
Expand Down
3 changes: 1 addition & 2 deletions python/ray/includes/libcoreworker.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,7 @@ cdef extern from "ray/core_worker/core_worker.h" nogil:
c_bool ref_counting_enabled
c_bool is_local_mode
int num_workers
(c_bool() nogil) kill_main
# ABCDEFG
(void() nogil) kill_main
CCoreWorkerOptions()

cdef cppclass CCoreWorkerProcess "ray::CoreWorkerProcess":
Expand Down
5 changes: 3 additions & 2 deletions src/ray/core_worker/core_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1326,6 +1326,7 @@ Status CoreWorker::ExecuteTask(const TaskSpecification &task_spec,
{
absl::MutexLock lock(&mutex_);
current_task_ = task_spec;
// Try to force kill if Cancellation RPC arrived before execution started.
TryForceKillTask();
}

Expand Down Expand Up @@ -1668,7 +1669,7 @@ void CoreWorker::TryForceKillTask() {
return;
}
if (main_thread_task_id_ == task_to_kill_) {
RAY_LOG(INFO) << "Force killing worker running: " << main_thread_task_id_;
RAY_LOG(INFO) << "Force killing a worker running " << main_thread_task_id_;
RAY_IGNORE_EXPR(local_raylet_client_->Disconnect());
if (options_.log_dir != "") {
RayLog::ShutDownRayLog();
Expand All @@ -1687,7 +1688,7 @@ void CoreWorker::HandleKillTask(const rpc::KillTaskRequest &request,
absl::MutexLock lock(&mutex_);
if (!request.force_kill()) {
if (main_thread_task_id_ == task_id) {
RAY_LOG(INFO) << "Soft killing worker running: " << main_thread_task_id_;
RAY_LOG(INFO) << "Interrupting a running task " << main_thread_task_id_;
options_.kill_main();
}
return;
Expand Down
3 changes: 2 additions & 1 deletion src/ray/core_worker/core_worker.h
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ struct CoreWorkerOptions {
/// Language worker callback to get the current call stack.
std::function<void(std::string *)> get_lang_stack;
// Function that tries to interrupt the currently running Python thread.
std::function<bool()> kill_main;
std::function<void()> kill_main;
/// Whether to enable object ref counting.
bool ref_counting_enabled;
/// Is local mode being used.
Expand Down Expand Up @@ -589,6 +589,7 @@ class CoreWorker : public rpc::CoreWorkerServiceHandler {
/// Stops the task associated with the given Object ID.
///
/// \param[in] object_id of the task to kill (must be a Non-Actor task)
/// \param[in] force_kill Whether to force kill a task by killing the worker.
/// \param[out] Status
Status KillTask(const ObjectID &object_id, bool force_kill);
/// Decrease the reference count for this actor. Should be called by the
Expand Down
2 changes: 1 addition & 1 deletion src/ray/core_worker/reference_count.cc
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ void ReferenceCounter::RemoveSubmittedTaskReferences(
<< argument_id;
return;
}
RAY_CHECK(it->second.submitted_task_ref_count > 0) << argument_id;
RAY_CHECK(it->second.submitted_task_ref_count > 0);
it->second.submitted_task_ref_count--;
if (release_lineage) {
if (it->second.lineage_ref_count > 0) {
Expand Down
10 changes: 4 additions & 6 deletions src/ray/core_worker/task_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ void TaskManager::CompletePendingTask(const TaskID &task_id,
ShutdownIfNeeded();
}

void TaskManager::MarkTaskCancelled(const TaskID &task_id, bool store_output) {
void TaskManager::MarkTaskCancelled(const TaskID &task_id) {
{
absl::MutexLock lock(&mu_);
auto it = submissible_tasks_.find(task_id);
Expand All @@ -212,9 +212,7 @@ void TaskManager::MarkTaskCancelled(const TaskID &task_id, bool store_output) {
it->second.num_retries_left = 0;
it->second.cancelled = true;
}
if (store_output) {
MarkPendingTaskFailed(task_id, GetTaskSpec(task_id), rpc::ErrorType::TASK_CANCELLED);
}
MarkPendingTaskFailed(task_id, GetTaskSpec(task_id), rpc::ErrorType::TASK_CANCELLED);
}

void TaskManager::PendingTaskFailed(const TaskID &task_id, rpc::ErrorType error_type,
Expand Down Expand Up @@ -379,8 +377,8 @@ void TaskManager::RemoveLineageReference(const ObjectID &object_id,
void TaskManager::MarkPendingTaskFailed(const TaskID &task_id,
const TaskSpecification &spec,
rpc::ErrorType error_type) {
RAY_LOG(ERROR) << "Treat task as failed. task_id: " << task_id
<< ", error_type: " << ErrorType_Name(error_type);
RAY_LOG(INFO) << "Treat task as failed. task_id: " << task_id
<< ", error_type: " << ErrorType_Name(error_type);
int64_t num_returns = spec.NumReturns();
for (int i = 0; i < num_returns; i++) {
const auto object_id = ObjectID::ForTaskReturn(
Expand Down
4 changes: 2 additions & 2 deletions src/ray/core_worker/task_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class TaskFinisherInterface {
virtual void PendingTaskFailed(const TaskID &task_id, rpc::ErrorType error_type,
Status *status = nullptr) = 0;

virtual void MarkTaskCancelled(const TaskID &task_id, bool store_output) = 0;
virtual void MarkTaskCancelled(const TaskID &task_id) = 0;

virtual void OnTaskDependenciesInlined(
const std::vector<ObjectID> &inlined_dependency_ids,
Expand Down Expand Up @@ -113,7 +113,7 @@ class TaskManager : public TaskFinisherInterface {
/// Cancels a task by treating it as failed and removes any remaining retries
///
/// \param[in] task_id The TaskId that is being cancelled
void MarkTaskCancelled(const TaskID &task_id, bool store_output);
void MarkTaskCancelled(const TaskID &task_id);

/// Return the spec for a pending task.
TaskSpecification GetTaskSpec(const TaskID &task_id) const;
Expand Down
17 changes: 8 additions & 9 deletions src/ray/core_worker/transport/direct_task_transport.cc
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ Status CoreWorkerDirectTaskSubmitter::KillTask(TaskSpecification task_spec,
task_spec.GetSchedulingClass(), task_spec.GetDependencies(),
task_spec.IsActorCreationTask() ? task_spec.ActorCreationId() : ActorID::Nil());
std::shared_ptr<rpc::CoreWorkerClientInterface> client = nullptr;
bool full_kill = false;
bool is_pending = false;
{
absl::MutexLock lock(&mu_);
auto scheduled_tasks = task_queues_.find(scheduling_key);
Expand All @@ -253,8 +253,8 @@ Status CoreWorkerDirectTaskSubmitter::KillTask(TaskSpecification task_spec,
spec != scheduled_tasks->second.end(); spec++) {
if (spec->TaskId() == task_spec.TaskId()) {
scheduled_tasks->second.erase(spec);
full_kill = true;
// Erase an empty queue
is_pending = true;

if (scheduled_tasks->second.empty()) {
task_queues_.erase(scheduling_key);
}
Expand All @@ -273,16 +273,15 @@ Status CoreWorkerDirectTaskSubmitter::KillTask(TaskSpecification task_spec,
}
}
}
task_finisher_->MarkTaskCancelled(task_spec.TaskId());
if (is_pending) {
task_finisher_->PendingTaskFailed(task_spec.TaskId(), rpc::ErrorType::TASK_CANCELLED);
}

if (client == nullptr) {
task_finisher_->MarkTaskCancelled(task_spec.TaskId(), true);
if (full_kill) {
task_finisher_->PendingTaskFailed(task_spec.TaskId(),
rpc::ErrorType::TASK_CANCELLED);
}
return Status::OK();
}

task_finisher_->MarkTaskCancelled(task_spec.TaskId(), false);
auto request = rpc::KillTaskRequest();
request.set_intended_task_id(task_spec.TaskId().Binary());
request.set_force_kill(force_kill);
Expand Down