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

feat: use DTO for NCNN init parameters #1147

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sileht
Copy link
Contributor

@sileht sileht commented Jan 21, 2021

feat: use DTO for NCNN init parameters

refactor(dto): example service_create+ncnn

The Service Create call works, but most parameters doesn't.

Supported parameters are only those declared in the DTO files.

To not update all input/output connector code. This PR just
convert the DTO into APIData to pass it to connector.

/!\ But beware if a parameters is not present in the DTO it will be
ignored and will not be present in the generated APIData passed to the
connectors. /!\

That's why we need first to fill the InputConnector and OutputConnector
with all possible option from all type and all backends.

cmake .. -D USE_HTTP_SERVER_OATPP=ON -D USE_HTTP_SERVER=OFF -D USE_NCNN=ON -D USE_CAFFE=OFF -DUSE_COMMAND_LINE=OFF

@sileht sileht changed the title Pull request for dto-ncnn feat: use DTO for NCNN init parameters Jan 21, 2021
@beniz
Copy link
Collaborator

beniz commented Jan 22, 2021

Thanks, sounds simple. What do think about putting the backend DTOs headers into src/backends/ncnn/dto for instance for ncnn instead of within the higher level http/ dir ?

@mergify
Copy link
Contributor

mergify bot commented Jan 22, 2021

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Jan 22, 2021
@mergify mergify bot removed the conflict label Jan 22, 2021
@sileht sileht force-pushed the dto-ncnn branch 4 times, most recently from bcf2ebb to 9d688cc Compare January 28, 2021 07:56
@beniz beniz added the kind:dto label Feb 11, 2021
@sileht sileht force-pushed the dto-ncnn branch 4 times, most recently from 109e8c4 to 45db8cd Compare February 12, 2021 15:14
@@ -312,7 +320,18 @@ TEST(ncnnapi, ocr)
+ ocr_repo
+ "\"},\"parameters\":{\"input\":{\"connector\":\"image\",\"ctc\":"
"true, \"height\":136,\"width\":220},\"mllib\":{\"nclasses\":69}}}";
std::string joutstr = japi.jrender(japi.service_create(sname, jstr));

auto service_create = dd::DTO::ServiceCreate::createShared();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way of creating the DTO object directly from the JSON string ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and that's a good idea to do it in tests, it will ensure the current json string still work as-is.

The Service Create call works, but most parameters doesn't.

Supported parameters are only those declared in the DTO files.

To not update all input/output connector code. This PR just
convert the DTO into APIData to pass it to connector.

/!\ But beware if a parameters is not present in the DTO it will be
ignored and will not be present in the generated APIData passed to the
connectors. /!\

That's why we need first to fill the InputConnector and OutputConnector
with all possible option from all type and all backends.

cmake .. -D USE_HTTP_SERVER_OATPP=ON -D USE_HTTP_SERVER=OFF -D USE_NCNN=ON -D USE_CAFFE=OFF -DUSE_COMMAND_LINE=OFF
@beniz beniz marked this pull request as draft March 24, 2021 09:57
@mergify
Copy link
Contributor

mergify bot commented Nov 13, 2021

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Nov 13, 2021
@mergify mergify bot removed the conflict label Sep 26, 2022
@mergify
Copy link
Contributor

mergify bot commented Sep 26, 2022

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Sep 26, 2022
@mergify mergify bot removed the conflict label Mar 16, 2023
@mergify
Copy link
Contributor

mergify bot commented Mar 16, 2023

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Mar 16, 2023
@mergify mergify bot removed the conflict label May 15, 2023
@mergify
Copy link
Contributor

mergify bot commented May 15, 2023

This pull request is now in conflict :(

@mergify mergify bot added the conflict label May 15, 2023
@mergify mergify bot removed the conflict label Jun 30, 2023
@mergify
Copy link
Contributor

mergify bot commented Jun 30, 2023

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Jun 30, 2023
@mergify mergify bot removed the conflict label Jan 8, 2024
Copy link
Contributor

mergify bot commented Jan 8, 2024

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants