Added handling of relative paths #2384
Conversation
bstrzele
force-pushed
the
llm_relative_paths
branch
from
0dfbb80
to
005161b
Compare
There was a problem hiding this comment.
Please update kwargs spec in the documentation:
https://github.com/openvinotoolkit/model_server/blob/main/docs/python_support/reference.md#parameters-and-return-value
| @@ -3,7 +3,7 @@ | |||
| "mediapipe_config_list": [ | |||
| { | |||
| "name":"python_model", | |||
| "graph_path":"/workspace/graph.pbtxt" | |||
| "graph_path":"./graph.pbtxt" | |||
There was a problem hiding this comment.
Will it work if we go only with "graph.pbtxt"?
There was a problem hiding this comment.
Yup, that works too
demos/python_demos/llm_text_generation/servable_unary/python_model/model.py
Show resolved
Hide resolved
| @@ -422,7 +422,7 @@ Status MediapipeGraphDefinition::initializeNodes() { | |||
| } | |||
|
|
|||
| std::shared_ptr<PythonNodeResources> nodeResources = nullptr; | |||
| Status status = PythonNodeResources::createPythonNodeResources(nodeResources, config.node(i), pythonBackend); | |||
| Status status = PythonNodeResources::createPythonNodeResources(nodeResources, config.node(i), pythonBackend, mgconfig.getBasePath()); | |||
There was a problem hiding this comment.
Is the base_path an absolute path to the directory with config.json or graph.pbtxt?
There was a problem hiding this comment.
mgconfig.getBasePath() return path to graph.pbtxt
| @@ -2260,7 +2260,7 @@ TEST_F(PythonFlowTest, FinalizePassTest) { | |||
| ASSERT_TRUE(::google::protobuf::TextFormat::ParseFromString(pbTxt, &config)); | |||
|
|
|||
| std::shared_ptr<PythonNodeResources> nodeResources = nullptr; | |||
| ASSERT_EQ(PythonNodeResources::createPythonNodeResources(nodeResources, config.node(0), getPythonBackend()), StatusCode::OK); | |||
| ASSERT_EQ(PythonNodeResources::createPythonNodeResources(nodeResources, config.node(0), getPythonBackend(), ""), StatusCode::OK); | |||
There was a problem hiding this comment.
Is empty string a possible value? If createPythonNodeResources is not called with empty string as a basePath in any real scenario then this setting does not provide valueable information and might be misleading.
There was a problem hiding this comment.
It is not possible in regular use case when pbtxt is located on file system. In case of these tests every value is equally valid, since there is no actual basePath in those scenarios.
There was a problem hiding this comment.
Okay, in that case can you add two tests for checking if base_path is passed as expected?
One with relative handler path, second with absolute and confirm that kwargs["base_path"] on the Python side is correct.
bstrzele
force-pushed
the
llm_relative_paths
branch
from
005161b
to
a65bb41
Compare
src/python/pythonnoderesources.cpp
Outdated
| @@ -112,24 +113,29 @@ void createOutputTagNameMapping(std::shared_ptr<PythonNodeResources>& nodeResour | |||
| } | |||
| } | |||
|
|
|||
| Status PythonNodeResources::createPythonNodeResources(std::shared_ptr<PythonNodeResources>& nodeResources, const ::mediapipe::CalculatorGraphConfig::Node& graphNodeConfig, PythonBackend* pythonBackend) { | |||
| Status PythonNodeResources::createPythonNodeResources(std::shared_ptr<PythonNodeResources>& nodeResources, const ::mediapipe::CalculatorGraphConfig::Node& graphNodeConfig, PythonBackend* pythonBackend, std::string basePath) { | |||
There was a problem hiding this comment.
To avoid ambiguity can we rename argument basePath to graphPath and then basePath could be used instead of parentPath. This way we instantly know that createPythonNodeResources gets path to the graph configuration, but preparePythonNodeInitializeArguments gets path to the handler file parent directory. Currently we use name basePath in more than one place, but it doesn't mean the same thing.
bstrzele
force-pushed
the
llm_token_reporting
branch
from
07bce23
to
0d92556
Compare
bstrzele
force-pushed
the
llm_relative_paths
branch
from
6a72534
to
cd57961
Compare
bstrzele
force-pushed
the
llm_relative_paths
branch
from
cd57961
to
8114e1a
Compare
src/python/pythonnoderesources.hpp
Outdated
| @@ -48,7 +48,7 @@ struct PythonNodeResources { | |||
| void finalize(); | |||
|
|
|||
| private: | |||
| static py::dict preparePythonNodeInitializeArguments(const ::mediapipe::CalculatorGraphConfig::Node& graphNodeConfig, std::string basePath); | |||
| static py::dict preparePythonNodeInitializeArguments(const ::mediapipe::CalculatorGraphConfig::Node& graphNodeConfig, std::string graphPath); | |||
There was a problem hiding this comment.
Actually here it would be basePath
No description provided.