Skip to content

Move ort flatbuffers helper functions and value info r/w functions into separated lib#5276

Merged
guoyu-wang merged 14 commits intomasterfrom
gwang-msft/ort_flatbuffers
Sep 25, 2020
Merged

Move ort flatbuffers helper functions and value info r/w functions into separated lib#5276
guoyu-wang merged 14 commits intomasterfrom
gwang-msft/ort_flatbuffers

Conversation

@guoyu-wang
Copy link
Contributor

@guoyu-wang guoyu-wang commented Sep 23, 2020

Description: Move ort flatbuffers helper functions and value info r/w functions into separated lib

Motivation and Context

  • Remove the dependency of graph/model if we want to get input/output info from ort model
  • Remove ort flatbuffers header from graph/model/ssession_state headers
  • Remove many static linked libs for onnxruntime_perf_test
  • Enable ort model support for full build onnx_test_runner/onnxruntime_perf_test

@guoyu-wang guoyu-wang requested a review from a team as a code owner September 23, 2020 23:54
@guoyu-wang guoyu-wang changed the title [WIP] Move most of the flatbuffer r/w functions into separated lib [WIP] Move ort flatbuffers helper functions and value info r/w functions into separated lib Sep 24, 2020
@guoyu-wang guoyu-wang changed the title [WIP] Move ort flatbuffers helper functions and value info r/w functions into separated lib Move ort flatbuffers helper functions and value info r/w functions into separated lib Sep 24, 2020
#include <core/common/status.h>
#include <core/graph/basic_types.h>
#include <core/session/onnxruntime_c_api.h>

Copy link
Contributor

@skottmckay skottmckay Sep 25, 2020

Choose a reason for hiding this comment

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

the C API is a really high level thing. why does a low level thing like this library have a dependency on it? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, this include is only for the "ORT_MUST_USE_RESULT" which is defined in the c_api


In reply to: 494752637 [](ancestors = 494752637)

endif()
target_include_directories(onnxruntime_flatbuffers PRIVATE ${ONNXRUNTIME_ROOT})
add_dependencies(onnxruntime_flatbuffers ${onnxruntime_EXTERNAL_DEPENDENCIES})
set_target_properties(onnxruntime_flatbuffers PROPERTIES FOLDER "ONNXRuntime")
Copy link
Contributor

@skottmckay skottmckay Sep 25, 2020

Choose a reason for hiding this comment

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

this has a dependency on the graph library doesn't it given flatbuffers_utils.h includes a header from there? #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

may not need a link dependency but would be good to be more explicit about the dependency on the include path


In reply to: 494752823 [](ancestors = 494752823)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Including core/graph/basic_types.h is only for the declaration of ONNX_NAMESPACE::ValueInfoProto, removed and add some forward decls


In reply to: 494759244 [](ancestors = 494759244,494752823)


for (const auto argdef : grad_argdefs) {
for (const auto& argdef : grad_argdefs) {
ONNX_NAMESPACE::TensorProto_DataType elem_type =
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 is a minor fix for a warning on latest Apple clang

skottmckay
skottmckay previously approved these changes Sep 25, 2020
Copy link
Contributor

@skottmckay skottmckay left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@skottmckay skottmckay left a comment

Choose a reason for hiding this comment

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

:shipit:

@guoyu-wang guoyu-wang merged commit 3a3f26f into master Sep 25, 2020
@guoyu-wang guoyu-wang deleted the gwang-msft/ort_flatbuffers branch September 25, 2020 12:36
@xkszltl
Copy link
Contributor

xkszltl commented Sep 27, 2020

@gwang-msft FYI this PR breaks master build.
3a3f26f#commitcomment-42736525

@guoyu-wang
Copy link
Contributor Author

@xkszltl, yes, this will cause problems, not sure why CI did not catch this.
What's you build command? I will fix this

@xkszltl
Copy link
Contributor

xkszltl commented Sep 27, 2020

I use cmake+ninja, maybe the generator is more sensitive than makefile.

@xkszltl
Copy link
Contributor

xkszltl commented Sep 27, 2020

This is the build arg: https://github.com/xkszltl/Roaster/blob/14e68367389ad0b8fd809a48b4b70a76f183f89e/pkgs/ort.sh#L75-L135
Most of it should be irrelevant to this issue.

@xkszltl
Copy link
Contributor

xkszltl commented Sep 27, 2020

There was another one, so it'll be great if it can be captured by CI:
#5024

@guoyu-wang
Copy link
Contributor Author

This #5306 should fix the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants