diff options
-rw-r--r-- | eval/src/apps/analyze_onnx_model/analyze_onnx_model.cpp | 62 | ||||
-rw-r--r-- | eval/src/tests/apps/analyze_onnx_model/CMakeLists.txt | 5 | ||||
-rw-r--r-- | eval/src/tests/apps/analyze_onnx_model/analyze_onnx_model_test.cpp | 63 | ||||
-rw-r--r-- | eval/src/tests/apps/eval_expr/CMakeLists.txt | 5 | ||||
-rw-r--r-- | eval/src/tests/apps/eval_expr/eval_expr_test.cpp | 5 | ||||
-rw-r--r-- | eval/src/vespa/eval/eval/test/test_io.cpp | 89 | ||||
-rw-r--r-- | eval/src/vespa/eval/eval/test/test_io.h | 17 | ||||
-rw-r--r-- | eval/src/vespa/eval/onnx/onnx_wrapper.cpp | 16 | ||||
-rw-r--r-- | eval/src/vespa/eval/onnx/onnx_wrapper.h | 1 | ||||
-rw-r--r-- | vespalib/src/vespa/vespalib/util/child_process.cpp | 20 | ||||
-rw-r--r-- | vespalib/src/vespa/vespalib/util/child_process.h | 13 |
11 files changed, 261 insertions, 35 deletions
diff --git a/eval/src/apps/analyze_onnx_model/analyze_onnx_model.cpp b/eval/src/apps/analyze_onnx_model/analyze_onnx_model.cpp index 506073ae8b3..868e9d036f1 100644 --- a/eval/src/apps/analyze_onnx_model/analyze_onnx_model.cpp +++ b/eval/src/apps/analyze_onnx_model/analyze_onnx_model.cpp @@ -20,6 +20,10 @@ using vespalib::FilePointer; using namespace vespalib::eval; using namespace vespalib::eval::test; +struct MyError { + vespalib::string msg; +}; + bool read_line(FilePointer &file, vespalib::string &line) { char line_buffer[1024]; char *res = fgets(line_buffer, sizeof(line_buffer), file.fp()); @@ -139,16 +143,50 @@ struct MakeInputType { } }; +vespalib::string make_bound_str(const std::map<vespalib::string,size_t> &bound) { + vespalib::string result; + if (!bound.empty()) { + for (const auto &[name, size]: bound) { + if (result.empty()) { + result.append(" ("); + } else { + result.append(","); + } + result.append(fmt("%s=%zu", name.c_str(), size)); + } + result.append(")"); + } + return result; +} + +void bind_input(Onnx::WirePlanner &planner, const Onnx::TensorInfo &input, const ValueType &type) { + auto bound = planner.get_bound_sizes(input); + if (!planner.bind_input_type(type, input)) { + auto bound_str = make_bound_str(bound); + throw MyError{fmt("incompatible type for input '%s': %s -> %s%s", + input.name.c_str(), type.to_spec().c_str(), input.type_as_string().c_str(), bound_str.c_str())}; + } +} + +ValueType make_output(const Onnx::WirePlanner &planner, const Onnx::TensorInfo &output) { + auto type = planner.make_output_type(output); + if (type.is_error()) { + throw MyError{fmt("unable to make compatible type for output '%s': %s -> error", + output.name.c_str(), output.type_as_string().c_str())}; + } + return type; +} + Onnx::WireInfo make_plan(Options &opts, const Onnx &model) { Onnx::WirePlanner planner; MakeInputType make_input_type(opts); for (const auto &input: model.inputs()) { auto type = make_input_type(input); - REQUIRE(planner.bind_input_type(type, input)); + bind_input(planner, input, type); } planner.prepare_output_types(model); for (const auto &output: model.outputs()) { - REQUIRE(!planner.make_output_type(output).is_error()); + make_output(planner, output); } return planner.get_wire_info(model); } @@ -189,7 +227,7 @@ int probe_types() { StdOut std_out; Slime params; if (!JsonFormat::decode(std_in, params)) { - return 3; + throw MyError{"invalid json"}; } Slime result; auto &root = result.setObject(); @@ -199,13 +237,20 @@ int probe_types() { for (size_t i = 0; i < model.inputs().size(); ++i) { auto spec = params["inputs"][model.inputs()[i].name].asString().make_string(); auto input_type = ValueType::from_spec(spec); - REQUIRE(!input_type.is_error()); - REQUIRE(planner.bind_input_type(input_type, model.inputs()[i])); + if (input_type.is_error()) { + if (!params["inputs"][model.inputs()[i].name].valid()) { + throw MyError{fmt("missing type for model input '%s'", + model.inputs()[i].name.c_str())}; + } else { + throw MyError{fmt("invalid type for model input '%s': '%s'", + model.inputs()[i].name.c_str(), spec.c_str())}; + } + } + bind_input(planner, model.inputs()[i], input_type); } planner.prepare_output_types(model); for (const auto &output: model.outputs()) { - auto output_type = planner.make_output_type(output); - REQUIRE(!output_type.is_error()); + auto output_type = make_output(planner, output); types.setString(output.name, output_type.to_spec()); } write_compact(result, std_out); @@ -253,6 +298,9 @@ int my_main(int argc, char **argv) { int main(int argc, char **argv) { try { return my_main(argc, argv); + } catch (const MyError &err) { + fprintf(stderr, "error: %s\n", err.msg.c_str()); + return 3; } catch (const std::exception &ex) { fprintf(stderr, "got exception: %s\n", ex.what()); return 2; diff --git a/eval/src/tests/apps/analyze_onnx_model/CMakeLists.txt b/eval/src/tests/apps/analyze_onnx_model/CMakeLists.txt index 7b70360a622..06241b0add6 100644 --- a/eval/src/tests/apps/analyze_onnx_model/CMakeLists.txt +++ b/eval/src/tests/apps/analyze_onnx_model/CMakeLists.txt @@ -4,6 +4,7 @@ vespa_add_executable(eval_analyze_onnx_model_test_app TEST analyze_onnx_model_test.cpp DEPENDS vespaeval + AFTER + eval_analyze_onnx_model_app ) -vespa_add_test(NAME eval_analyze_onnx_model_test_app COMMAND eval_analyze_onnx_model_test_app - DEPENDS eval_analyze_onnx_model_test_app eval_analyze_onnx_model_app) +vespa_add_test(NAME eval_analyze_onnx_model_test_app COMMAND eval_analyze_onnx_model_test_app) diff --git a/eval/src/tests/apps/analyze_onnx_model/analyze_onnx_model_test.cpp b/eval/src/tests/apps/analyze_onnx_model/analyze_onnx_model_test.cpp index 58cc7c22358..72ef0346ea3 100644 --- a/eval/src/tests/apps/analyze_onnx_model/analyze_onnx_model_test.cpp +++ b/eval/src/tests/apps/analyze_onnx_model/analyze_onnx_model_test.cpp @@ -17,10 +17,12 @@ vespalib::string get_source_dir() { } vespalib::string source_dir = get_source_dir(); vespalib::string probe_model = source_dir + "/../../tensor/onnx_wrapper/probe_model.onnx"; +vespalib::string simple_model = source_dir + "/../../tensor/onnx_wrapper/simple.onnx"; +vespalib::string dynamic_model = source_dir + "/../../tensor/onnx_wrapper/dynamic.onnx"; //----------------------------------------------------------------------------- -TEST_F("require that output types can be probed", ServerCmd(probe_cmd, true)) { +TEST_F("require that output types can be probed", ServerCmd(probe_cmd)) { Slime params; params.setObject(); params.get().setString("model", probe_model); @@ -32,6 +34,65 @@ TEST_F("require that output types can be probed", ServerCmd(probe_cmd, true)) { EXPECT_EQUAL(result["outputs"]["out1"].asString().make_string(), vespalib::string("tensor<float>(d0[2],d1[3])")); EXPECT_EQUAL(result["outputs"]["out2"].asString().make_string(), vespalib::string("tensor<float>(d0[2],d1[3])")); EXPECT_EQUAL(result["outputs"]["out3"].asString().make_string(), vespalib::string("tensor<float>(d0[2],d1[3])")); + EXPECT_EQUAL(f1.shutdown(), 0); +} + +//----------------------------------------------------------------------------- + +TEST_F("test error: invalid json", ServerCmd(probe_cmd, ServerCmd::capture_stderr_tag())) { + auto out = f1.write_then_read_all("this is not valid json...\n"); + EXPECT_TRUE(out.find("invalid json") < out.size()); + EXPECT_EQUAL(f1.shutdown(), 3); +} + +TEST_F("test error: missing input type", ServerCmd(probe_cmd, ServerCmd::capture_stderr_tag())) { + Slime params; + params.setObject(); + params.get().setString("model", simple_model); + params.get().setObject("inputs"); + auto out = f1.write_then_read_all(params.toString()); + EXPECT_TRUE(out.find("missing type") < out.size()); + EXPECT_EQUAL(f1.shutdown(), 3); +} + +TEST_F("test error: invalid input type", ServerCmd(probe_cmd, ServerCmd::capture_stderr_tag())) { + Slime params; + params.setObject(); + params.get().setString("model", simple_model); + params.get().setObject("inputs"); + params["inputs"].setString("query_tensor", "bogus type string"); + params["inputs"].setString("attribute_tensor", "tensor<float>(x[4],y[1])"); + params["inputs"].setString("bias_tensor", "tensor<float>(x[1],y[1])"); + auto out = f1.write_then_read_all(params.toString()); + EXPECT_TRUE(out.find("invalid type") < out.size()); + EXPECT_EQUAL(f1.shutdown(), 3); +} + +TEST_F("test error: incompatible input type", ServerCmd(probe_cmd, ServerCmd::capture_stderr_tag())) { + Slime params; + params.setObject(); + params.get().setString("model", simple_model); + params.get().setObject("inputs"); + params["inputs"].setString("query_tensor", "tensor<float>(x[1],y[5])"); + params["inputs"].setString("attribute_tensor", "tensor<float>(x[4],y[1])"); + params["inputs"].setString("bias_tensor", "tensor<float>(x[1],y[1])"); + auto out = f1.write_then_read_all(params.toString()); + EXPECT_TRUE(out.find("incompatible type") < out.size()); + EXPECT_EQUAL(f1.shutdown(), 3); +} + +TEST_F("test error: symbolic size mismatch", ServerCmd(probe_cmd, ServerCmd::capture_stderr_tag())) { + Slime params; + params.setObject(); + params.get().setString("model", dynamic_model); + params.get().setObject("inputs"); + params["inputs"].setString("query_tensor", "tensor<float>(x[1],y[4])"); + params["inputs"].setString("attribute_tensor", "tensor<float>(x[4],y[1])"); + params["inputs"].setString("bias_tensor", "tensor<float>(x[2],y[1])"); + auto out = f1.write_then_read_all(params.toString()); + EXPECT_TRUE(out.find("incompatible type") < out.size()); + EXPECT_TRUE(out.find("batch=1") < out.size()); + EXPECT_EQUAL(f1.shutdown(), 3); } //----------------------------------------------------------------------------- diff --git a/eval/src/tests/apps/eval_expr/CMakeLists.txt b/eval/src/tests/apps/eval_expr/CMakeLists.txt index e36c5aafe23..c36cf33e2b2 100644 --- a/eval/src/tests/apps/eval_expr/CMakeLists.txt +++ b/eval/src/tests/apps/eval_expr/CMakeLists.txt @@ -4,6 +4,7 @@ vespa_add_executable(eval_eval_expr_test_app TEST eval_expr_test.cpp DEPENDS vespaeval + AFTER + eval_eval_expr_app ) -vespa_add_test(NAME eval_eval_expr_test_app COMMAND eval_eval_expr_test_app - DEPENDS eval_eval_expr_test_app eval_eval_expr_app) +vespa_add_test(NAME eval_eval_expr_test_app COMMAND eval_eval_expr_test_app) diff --git a/eval/src/tests/apps/eval_expr/eval_expr_test.cpp b/eval/src/tests/apps/eval_expr/eval_expr_test.cpp index 3834ea0e4cc..fd103829618 100644 --- a/eval/src/tests/apps/eval_expr/eval_expr_test.cpp +++ b/eval/src/tests/apps/eval_expr/eval_expr_test.cpp @@ -56,7 +56,7 @@ Result::~Result() = default; struct Server : public ServerCmd { TimeBomb time_bomb; - Server() : ServerCmd(server_cmd, true), time_bomb(60) {} + Server() : ServerCmd(server_cmd), time_bomb(60) {} Result eval(const vespalib::string &expr, const vespalib::string &name = {}, bool verbose = false) { Slime req; auto &obj = req.setObject(); @@ -70,6 +70,9 @@ struct Server : public ServerCmd { Slime reply = invoke(req); return {reply.get()}; } + ~Server() { + EXPECT_EQUAL(shutdown(), 0); + } }; //----------------------------------------------------------------------------- diff --git a/eval/src/vespa/eval/eval/test/test_io.cpp b/eval/src/vespa/eval/eval/test/test_io.cpp index ed7723f0748..044b6779431 100644 --- a/eval/src/vespa/eval/eval/test/test_io.cpp +++ b/eval/src/vespa/eval/eval/test/test_io.cpp @@ -120,45 +120,104 @@ ChildOut::evict(size_t bytes) //----------------------------------------------------------------------------- void -ServerCmd::maybe_dump_message(const char *prefix, const Slime &slime) +ServerCmd::maybe_close() { - if (_verbose) { - SimpleBuffer buf; - slime::JsonFormat::encode(slime, buf, false); - auto str = buf.get().make_string(); - fprintf(stderr, "%s%s: %s", prefix, _basename.c_str(), str.c_str()); + if (!_closed) { + _child.close(); + _closed = true; } } -ServerCmd::ServerCmd(vespalib::string cmd, bool verbose) +void +ServerCmd::maybe_exit() +{ + if (!_exited) { + read_until_eof(_child_stdout); + assert(_child.wait()); + assert(!_child.running()); + _exit_code = _child.getExitCode(); + _exited = true; + } +} + +void +ServerCmd::dump_string(const char *prefix, const vespalib::string &str) +{ + fprintf(stderr, "%s%s: '%s'\n", prefix, _basename.c_str(), str.c_str()); +} + +void +ServerCmd::dump_message(const char *prefix, const Slime &slime) +{ + SimpleBuffer buf; + slime::JsonFormat::encode(slime, buf, false); + auto str = buf.get().make_string(); + fprintf(stderr, "%s%s: %s", prefix, _basename.c_str(), str.c_str()); +} + +ServerCmd::ServerCmd(vespalib::string cmd) : _child(cmd.c_str()), _child_stdin(_child), _child_stdout(_child), _basename(fs::path(cmd).filename()), - _verbose(verbose) + _closed(false), + _exited(false), + _exit_code(31212) +{ +} + +ServerCmd::ServerCmd(vespalib::string cmd, capture_stderr_tag) + : _child(cmd.c_str(), ChildProcess::capture_stderr_tag()), + _child_stdin(_child), + _child_stdout(_child), + _basename(fs::path(cmd).filename()), + _closed(false), + _exited(false), + _exit_code(31212) { } ServerCmd::~ServerCmd() { - _child.close(); - read_until_eof(_child_stdout); - assert(_child.wait()); - assert(!_child.running()); - assert(!_child.failed()); + maybe_close(); + maybe_exit(); } Slime ServerCmd::invoke(const Slime &req) { - maybe_dump_message("request --> ", req); + dump_message("request --> ", req); write_compact(req, _child_stdin); Slime reply; REQUIRE(JsonFormat::decode(_child_stdout, reply)); - maybe_dump_message("reply <-- ", reply); + dump_message("reply <-- ", reply); return reply; } +vespalib::string +ServerCmd::write_then_read_all(const vespalib::string &input) +{ + vespalib::string result; + dump_string("input --> ", input); + memcpy(_child_stdin.reserve(input.size()).data, input.data(), input.size()); + _child_stdin.commit(input.size()); + maybe_close(); + for (auto mem = _child_stdout.obtain(); mem.size > 0; mem = _child_stdout.obtain()) { + result.append(mem.data, mem.size); + _child_stdout.evict(mem.size); + } + dump_string("output <-- ", result); + return result; +} + +int +ServerCmd::shutdown() +{ + maybe_close(); + maybe_exit(); + return _exit_code; +} + //----------------------------------------------------------------------------- bool diff --git a/eval/src/vespa/eval/eval/test/test_io.h b/eval/src/vespa/eval/eval/test/test_io.h index cef6cd6ae40..62fd6588780 100644 --- a/eval/src/vespa/eval/eval/test/test_io.h +++ b/eval/src/vespa/eval/eval/test/test_io.h @@ -73,13 +73,24 @@ private: ChildIn _child_stdin; ChildOut _child_stdout; vespalib::string _basename; - bool _verbose; + bool _closed; + bool _exited; + int _exit_code; + + void maybe_close(); + void maybe_exit(); + + void dump_string(const char *prefix, const vespalib::string &str); + void dump_message(const char *prefix, const Slime &slime); - void maybe_dump_message(const char *prefix, const Slime &slime); public: - ServerCmd(vespalib::string cmd, bool verbose); + struct capture_stderr_tag{}; + ServerCmd(vespalib::string cmd); + ServerCmd(vespalib::string cmd, capture_stderr_tag); ~ServerCmd(); Slime invoke(const Slime &req); + vespalib::string write_then_read_all(const vespalib::string &input); + int shutdown(); }; /** diff --git a/eval/src/vespa/eval/onnx/onnx_wrapper.cpp b/eval/src/vespa/eval/onnx/onnx_wrapper.cpp index 54c4b863e35..ed38a385d25 100644 --- a/eval/src/vespa/eval/onnx/onnx_wrapper.cpp +++ b/eval/src/vespa/eval/onnx/onnx_wrapper.cpp @@ -382,6 +382,22 @@ Onnx::WirePlanner::bind_input_type(const ValueType &vespa_in, const TensorInfo & return true; } +std::map<vespalib::string,size_t> +Onnx::WirePlanner::get_bound_sizes(const TensorInfo &onnx_in) const +{ + std::map<vespalib::string,size_t> result; + for (const auto &dim: onnx_in.dimensions) { + if (dim.is_symbolic()) { + auto pos = _symbolic_sizes.find(dim.name); + if (pos != _symbolic_sizes.end()) { + assert(pos->second != 0); + result.emplace(dim.name, pos->second); + } + } + } + return result; +} + void Onnx::WirePlanner::prepare_output_types(const Onnx &model) { diff --git a/eval/src/vespa/eval/onnx/onnx_wrapper.h b/eval/src/vespa/eval/onnx/onnx_wrapper.h index 678a4e40563..31421019e3c 100644 --- a/eval/src/vespa/eval/onnx/onnx_wrapper.h +++ b/eval/src/vespa/eval/onnx/onnx_wrapper.h @@ -94,6 +94,7 @@ public: ~WirePlanner(); static CellType best_cell_type(Onnx::ElementType type); bool bind_input_type(const ValueType &vespa_in, const TensorInfo &onnx_in); + std::map<vespalib::string,size_t> get_bound_sizes(const TensorInfo &onnx_in) const; void prepare_output_types(const Onnx &model); ValueType make_output_type(const TensorInfo &onnx_out) const; WireInfo get_wire_info(const Onnx &model) const; diff --git a/vespalib/src/vespa/vespalib/util/child_process.cpp b/vespalib/src/vespa/vespalib/util/child_process.cpp index 7193a445f9a..694bed60f1b 100644 --- a/vespalib/src/vespa/vespalib/util/child_process.cpp +++ b/vespalib/src/vespa/vespalib/util/child_process.cpp @@ -67,7 +67,9 @@ ChildProcess::Reader::OnReceiveData(const void *data, size_t length) return; } if (buf == nullptr) { // EOF - _gotEOF = true; + if (--_num_streams == 0) { + _gotEOF = true; + } } else { _queue.push(std::string(buf, length)); } @@ -107,11 +109,12 @@ ChildProcess::Reader::updateEOF() } -ChildProcess::Reader::Reader() +ChildProcess::Reader::Reader(int num_streams) : _lock(), _cond(), _queue(), _data(), + _num_streams(num_streams), _gotEOF(false), _waitCnt(0), _readEOF(false) @@ -203,7 +206,7 @@ ChildProcess::checkProc() ChildProcess::ChildProcess(const char *cmd) - : _reader(), + : _reader(1), _proc(cmd, true, &_reader), _running(false), _failed(false), @@ -213,6 +216,17 @@ ChildProcess::ChildProcess(const char *cmd) _failed = !_running; } +ChildProcess::ChildProcess(const char *cmd, capture_stderr_tag) + : _reader(2), + _proc(cmd, true, &_reader, &_reader), + _running(false), + _failed(false), + _exitCode(-918273645) +{ + _running = _proc.CreateWithShell(); + _failed = !_running; +} + ChildProcess::~ChildProcess() = default; diff --git a/vespalib/src/vespa/vespalib/util/child_process.h b/vespalib/src/vespa/vespalib/util/child_process.h index 646a2c7c6c9..877c56a8cb1 100644 --- a/vespalib/src/vespa/vespalib/util/child_process.h +++ b/vespalib/src/vespa/vespalib/util/child_process.h @@ -28,6 +28,7 @@ private: std::condition_variable _cond; std::queue<std::string> _queue; std::string _data; + int _num_streams; bool _gotEOF; int _waitCnt; bool _readEOF; @@ -38,7 +39,7 @@ private: void updateEOF(); public: - Reader(); + Reader(int num_streams); ~Reader() override; uint32_t read(char *buf, uint32_t len, int msTimeout); @@ -57,6 +58,7 @@ private: public: ChildProcess(const ChildProcess &) = delete; ChildProcess &operator=(const ChildProcess &) = delete; + struct capture_stderr_tag{}; /** * @brief Run a child process @@ -66,6 +68,15 @@ public: **/ explicit ChildProcess(const char *cmd); + /** + * @brief Run a child process + * + * Starts a process running the given command. stderr is + * redirected into stdout. + * @param cmd A shell command line to run + **/ + explicit ChildProcess(const char *cmd, capture_stderr_tag); + /** @brief destructor doing cleanup if needed */ ~ChildProcess(); |