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

[ml service] Move the duplicated structure and functions of the extension_service and training_offloading_services to ml_service. #573

Merged
merged 2 commits into from
Nov 11, 2024

Conversation

songgot
Copy link
Contributor

@songgot songgot commented Oct 23, 2024

[ml service] Code clean & Refactoring

  • Training offloading start function is separated into prepare_sender and preapre_receiver
  • Change mix used variable name
  • ml_pipeline_start is commonly used in start functions

[ml service] Move the duplicated structure and functions of the extention_service and training_offloading_services to ml_service.

  • Remove _ml_extension_invoke_new_data(), _invoke_event_new_data()
  • Remove ml_extension_node_type_e, ml_extension_node_info_s
  • Remove ml_training_offloading_node_type_e, ml_training_offloading_node_info_s
  • Remove _ml_extension_pipeline_sink_cb(), _pipeline_sink_cb()
  • Create _ml_service_invoke_event_new_data()
  • Create ml_service_node_type_e, ml_service_node_info_s
  • Create _ml_service_pipeline_sink_cb()
  • Code clean: Change mix used variable name

Pull Up Method : https://refactoring.guru/ko/pull-up-method
Pull Up Field : https://refactoring.guru/ko/pull-up-field
Duplicate Code: https://refactoring.guru/ko/smells/duplicate-code

@taos-ci
Copy link
Collaborator

taos-ci commented Oct 23, 2024

📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #573. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/.

@songgot songgot marked this pull request as draft October 23, 2024 06:34
@taos-ci
Copy link
Collaborator

taos-ci commented Oct 29, 2024

:octocat: cibot: @songgot, The last line of a text file must have a newline character. Please append a new line at the end of the line in c/src/ml-api-service.c.

@songgot songgot changed the title [ml service] Separate start function according to the tasks [ml service] Refect and clean the code of training offloading service Oct 29, 2024
@taos-ci
Copy link
Collaborator

taos-ci commented Oct 29, 2024

To contributor, We have used 'Signed-off-by:' notation by default to handle the license issues, that result from contributors. Note that 'Is there a Signed-off-by line?' is important because lawyers tell us we must have to it to cleanly maintain the open-source license issues even though it has nothing to do with the code itself.

@songgot songgot changed the title [ml service] Refect and clean the code of training offloading service [ml service] Refactor and clean the code of training offloading service Oct 30, 2024
@songgot songgot force-pushed the dev_training_offloading branch 2 times, most recently from 099bb0d to 0296526 Compare October 30, 2024 05:21
@taos-ci
Copy link
Collaborator

taos-ci commented Oct 30, 2024

:octocat: cibot: @songgot, The last line of a text file must have a newline character. Please append a new line at the end of the line in c/src/ml-api-service-training-offloading.c.

@taos-ci
Copy link
Collaborator

taos-ci commented Oct 31, 2024

:octocat: cibot: @songgot, A builder checker could not be completed because one of the checkers is not completed. In order to find out a reason, please go to http://ci.nnstreamer.ai/nnstreamer-api/ci/repo-workers/pr-checker/573-202410311110060.56479692459106-53309baeb2f84709dd33158db407346481fa1632/.

@taos-ci
Copy link
Collaborator

taos-ci commented Oct 31, 2024

To contributor, We have used 'Signed-off-by:' notation by default to handle the license issues, that result from contributors. Note that 'Is there a Signed-off-by line?' is important because lawyers tell us we must have to it to cleanly maintain the open-source license issues even though it has nothing to do with the code itself.

@taos-ci
Copy link
Collaborator

taos-ci commented Oct 31, 2024

To contributor, We have used 'Signed-off-by:' notation by default to handle the license issues, that result from contributors. Note that 'Is there a Signed-off-by line?' is important because lawyers tell us we must have to it to cleanly maintain the open-source license issues even though it has nothing to do with the code itself.

@songgot songgot marked this pull request as ready for review October 31, 2024 04:28
c/src/ml-api-service.c Outdated Show resolved Hide resolved
- Training offloading start function is separated into prepare_sender and preapre_receiver
- Change mix used variable name
- ml_pipeline_start is commonly used in start functions

Signed-off-by: hyunil park <[email protected]>
@songgot songgot force-pushed the dev_training_offloading branch 2 times, most recently from 5449eea to b0c5350 Compare November 5, 2024 04:53
@songgot songgot changed the title [ml service] Refactor and clean the code of training offloading service [ml service] Move the duplicated structure and functions of the extension_service Nov 5, 2024
Copy link
Collaborator

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@songgot, 💯 All CI checkers are successfully verified. Thanks.

@songgot songgot force-pushed the dev_training_offloading branch 2 times, most recently from 8206a1f to 17991a2 Compare November 5, 2024 23:55
Copy link
Collaborator

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@songgot, 💯 All CI checkers are successfully verified. Thanks.

Copy link
Member

@wooksong wooksong left a comment

Choose a reason for hiding this comment

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

Overall, comments that describe the functions are required to be revised.

…sion_service

 and training_offloading_services to ml_service.

- Remove _ml_extension_invoke_new_data(), _invoke_event_new_data()
- Remove ml_extension_node_type_e, ml_extension_node_info_s
- Remove ml_training_offloading_node_type_e, ml_training_offloading_node_info_s
- Remove _ml_extension_pipeline_sink_cb(), _pipeline_sink_cb()
- Create _ml_service_invoke_event_new_data()
- Create ml_service_node_type_e, ml_service_node_info_s
- Create _ml_service_pipeline_sink_cb()
- Code clean: Change mix used variable name

Signed-off-by: hyunil park <[email protected]>
@songgot
Copy link
Contributor Author

songgot commented Nov 7, 2024

Overall, comments that describe the functions are required to be revised.
Thank you for taking the time. I have revised the briefing you mentioned. 🙏

Copy link
Collaborator

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@songgot, 💯 All CI checkers are successfully verified. Thanks.

Copy link
Member

@gichan-jang gichan-jang left a comment

Choose a reason for hiding this comment

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

LGTM

@jaeyun-jung jaeyun-jung merged commit 6d0acf3 into nnstreamer:main Nov 11, 2024
37 checks passed
@songgot songgot changed the title [ml service] Move the duplicated structure and functions of the extension_service [ml service] Move the duplicated structure and functions of the extension_service and training_offloading_services to ml_service. Nov 15, 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.

5 participants