Skip to content

server : add handle_slot_type lambda #11535

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

Closed
wants to merge 7 commits into from
Closed

server : add handle_slot_type lambda #11535

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c5825e7
server : add handle_slot_type lambda
danbev Jan 31, 2025
7c34af4
squash! server : add handle_slot_type lambda
danbev Jan 31, 2025
2d2d076
server : extract handle_slot_erase to handle_slot_impl
danbev Jan 31, 2025
6cc2956
server : add server_task_type field to server_task_result
danbev Jan 31, 2025
0e09ec8
squash! server : add server_task_type field to server_task_result
danbev Jan 31, 2025
c6f6579
squash! server : add server_task_type field to server_task_result
danbev Jan 31, 2025
bfc342c
squash! server : add server_task_type field to server_task_result
danbev Feb 3, 2025
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
40 changes: 11 additions & 29 deletions examples/server/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3665,7 +3665,8 @@ int main(int argc, char ** argv) {
res.status = 200; // HTTP OK
};

const auto handle_slots_save = [&ctx_server, &res_error, &res_ok, &params](const httplib::Request & req, httplib::Response & res, int id_slot) {
const auto handle_slot_type = [&ctx_server, &res_error, &res_ok, &params](const httplib::Request & req,
httplib::Response & res, int id_slot, server_task_type type) {
json request_data = json::parse(req.body);
std::string filename = request_data.at("filename");
if (!fs_validate_filename(filename)) {
Expand All @@ -3674,7 +3675,7 @@ int main(int argc, char ** argv) {
}
std::string filepath = params.slot_save_path + filename;

server_task task(SERVER_TASK_TYPE_SLOT_SAVE);
server_task task(type);
task.id = ctx_server.queue_tasks.get_new_id();
task.slot_action.slot_id = id_slot;
task.slot_action.filename = filename;
Expand All @@ -3691,37 +3692,18 @@ int main(int argc, char ** argv) {
return;
}

if (type == SERVER_TASK_TYPE_SLOT_SAVE) {
GGML_ASSERT(dynamic_cast<server_task_result_slot_save_load*>(result.get()) != nullptr);
Copy link
Collaborator

@ngxson ngxson Jan 31, 2025

Choose a reason for hiding this comment

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

missing check for SERVER_TASK_TYPE_SLOT_RESTORE

Copy link
Collaborator

@ngxson ngxson Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

btw I'm not sure if we can use template here to avoid having a long list of if..else

Copy link
Member Author

@danbev danbev Jan 31, 2025

Choose a reason for hiding this comment

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

I'll take a look at this 👍

Copy link
Member Author

@danbev danbev Jan 31, 2025

Choose a reason for hiding this comment

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

I've added a suggestion for this in 6cc2956.

This contain an addition of a field, server_task_type, and a virtual function in server_task_result. The idea being that the dynamic cast like this can be avoided:

GGML_ASSERT(dynamic_cast<server_task_result_slot_save_load*>(result.get()) != nullptr);

In favor of checks like this:

        GGML_ASSERT(result.get() != nullptr);                                       
        GGML_ASSERT(result.get()->get_server_task_type() == type);

If this looks like alright then I can update the other places in server.cpp which also have similar dynamic_casts.

}
res_ok(res, result->to_json());
};

const auto handle_slots_restore = [&ctx_server, &res_error, &res_ok, &params](const httplib::Request & req, httplib::Response & res, int id_slot) {
json request_data = json::parse(req.body);
std::string filename = request_data.at("filename");
if (!fs_validate_filename(filename)) {
res_error(res, format_error_response("Invalid filename", ERROR_TYPE_INVALID_REQUEST));
return;
}
std::string filepath = params.slot_save_path + filename;

server_task task(SERVER_TASK_TYPE_SLOT_RESTORE);
task.id = ctx_server.queue_tasks.get_new_id();
task.slot_action.slot_id = id_slot;
task.slot_action.filename = filename;
task.slot_action.filepath = filepath;

ctx_server.queue_results.add_waiting_task_id(task.id);
ctx_server.queue_tasks.post(task);

server_task_result_ptr result = ctx_server.queue_results.recv(task.id);
ctx_server.queue_results.remove_waiting_task_id(task.id);

if (result->is_error()) {
res_error(res, result->to_json());
return;
}
const auto handle_slots_save = [&handle_slot_type](const httplib::Request & req, httplib::Response & res, int id_slot) {
handle_slot_type(req, res, id_slot, SERVER_TASK_TYPE_SLOT_SAVE);
};

GGML_ASSERT(dynamic_cast<server_task_result_slot_save_load*>(result.get()) != nullptr);
res_ok(res, result->to_json());
const auto handle_slots_restore = [&handle_slot_type](const httplib::Request & req, httplib::Response & res, int id_slot) {
handle_slot_type(req, res, id_slot, SERVER_TASK_TYPE_SLOT_RESTORE);
};

const auto handle_slots_erase = [&ctx_server, &res_error, &res_ok](const httplib::Request & /* req */, httplib::Response & res, int id_slot) {
Copy link
Collaborator

@ngxson ngxson Jan 31, 2025

Choose a reason for hiding this comment

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

IMO we should also extend the function to be reused by handle_slots_erase. The only thing need to adapt is that SERVER_TASK_TYPE_SLOT_ERASE ignores filename

Copy link
Member Author

@danbev danbev Jan 31, 2025

Choose a reason for hiding this comment

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

I'll take a look at this too.

Copy link
Member Author

@danbev danbev Jan 31, 2025

Choose a reason for hiding this comment

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

I've update this in 2d2d076

Expand Down
Loading