Cmake builds libgit2 and --pull_hf_mode for ovms #3105

rasapala · 2025-03-04T15:52:30Z

🛠 Summary

JIRA CVS-163308
Current usage:
bazel-bin/src/ovms --pull_hf_model --source_model OpenVINO/TinyLlama-1.1B-Chat-v1.0-int8-ov --repo_path /download

🧪 Checklist

Unit tests added.
The documentation updated.
Change follows security best practices.
``

…08_libgt2

atobiszei · 2025-03-07T09:48:25Z

src/server.cpp

-        parser.prepare(&serverSettings, &modelsSettings);
+        parser.prepare(&serverSettings, &modelsSettings, &hfDownloader);
+
+        if (hfDownloader.pull_hf_model) {


Shouldn't this be responsibility of ServableModule?

I think we should separate the server mode and hf download mode.

third_party/libgt2/libgt2_engine.bzl

…t/model_server into CVS-163308_libgt2

…08_libgt2

dtrawins · 2025-04-17T14:53:52Z

create_package.sh

@@ -84,6 +84,11 @@ patchelf --debug --set-rpath '$ORIGIN' /ovms_release/lib/libopenvino.so
 patchelf --debug --set-rpath '$ORIGIN' /ovms_release/lib/lib*plugin.so
 if [ -f  /ovms_release/lib/libopenvino_nvidia_gpu_plugin.so ] && [ "$BASE_OS" != "redhat" ]; then patchelf  --replace-needed libcutensor.so.1 /usr/lib/x86_64-linux-gnu/libcutensor/11/libcutensor.so.1 /ovms_release/lib/libopenvino_nvidia_gpu_plugin.so ; fi

+wget https://github.com/git-lfs/git-lfs/releases/download/v3.6.1/git-lfs-linux-arm64-v3.6.1.tar.gz


this is duplicated with dockerfile

dtrawins · 2025-04-17T14:56:04Z

src/cli_parser.cpp

@@ -126,6 +127,20 @@ void CLIParser::parse(int argc, char** argv) {
                "Absolute path to json configuration file",
                cxxopts::value<std::string>(), "CONFIG_PATH");

+        options->add_options("pull hf model")
+            ("pull_hf_model",


pull would be simpler and in like with ollama

dtrawins · 2025-04-17T14:56:46Z

src/cli_parser.cpp

+                "HF source model path",
+                cxxopts::value<std::string>(),
+                "HF_SOURCE")
+            ("repo_path",


what is repo path?

destination path for git clone. Needed for testing in current status.

This will be changed in implementation of other tasks.

atobiszei · 2025-04-17T13:42:09Z

src/hf_pull_model_module.hpp

+
+class HfPullModelModule : public Module {
+protected:
+    mutable HFSettingsImpl hfSettings;


Why mutable?

atobiszei · 2025-04-17T13:42:29Z

src/hf_pull_model_module.cpp

@@ -0,0 +1,78 @@
+//***************************************************************************
+// Copyright 2022 Intel Corporation


atobiszei · 2025-04-22T08:29:51Z

src/BUILD

@@ -2712,6 +2715,7 @@ cc_test(
        "test/pipelinedefinitionstatus_test.cpp",
        "test/predict_validation_test.cpp",
        "test/prediction_service_test.cpp",
+        "test/pull_hf_model_test.cpp",


yes but it will rebuild unnecessarily if any of ovms_tests dependency is changed

atobiszei · 2025-04-22T08:35:29Z

external/lfs.patch

+	git_process_free(process);
+	git__free(payload);
+	return;
+
+on_error:
+	git_process_free(process);
+	git__free(payload);
+	return;
+}


Suggested change

+ git_process_free(process);

+ git__free(payload);

+ return;

+

+on_error:

+ git_process_free(process);

+ git__free(payload);

+ return;

+}

+on_exit:

+ git_process_free(process);

+ git__free(payload);

+ return;

+}

You will need to rename on_error everywhere as well.

on_error is standard label in this library.

atobiszei · 2025-04-22T08:37:24Z

external/lfs.patch

+	git_process_free(process);
+	git__free(payload);
+	return;
+
+on_error:
+	git_process_free(process);
+	git__free(payload);
+	return;
+}


Suggested change

+ git_process_free(process);

+ git__free(payload);

+ return;

+

+on_error:

+ git_process_free(process);

+ git__free(payload);

+ return;

+}

+on_exit_:

+ git_process_free(process);

+ git__free(payload);

+ return;

+}

on_error: is the library convention.

atobiszei · 2025-04-22T08:40:00Z

src/config.cpp

@@ -30,6 +30,7 @@

 #include "capi_frontend/server_settings.hpp"
 #include "cli_parser.hpp"
+#include "libgt2/libgt2.hpp"


atobiszei · 2025-04-22T08:40:29Z

src/config.hpp

@@ -48,7 +48,7 @@ class Config {
    ServerSettingsImpl serverSettings;

 public:
-    ServerSettingsImpl getServerSettings() {
+    const ServerSettingsImpl getServerSettings() const {


Since we return const why no const&?

atobiszei · 2025-04-22T08:42:39Z

src/hf_pull_model_module.cpp

+        return StatusCode::HF_FAILED_TO_INIT_LIBGIT2;
+    }
+    std::unique_ptr<HfDownloader> hfDownloader = std::make_unique<HfDownloader>(this->hfSettings.sourceModel, this->hfSettings.repoPath, this->hfSettings.pullHfModelMode);
+    // TODO: Do we want to set timeout for this operation ?


Do we have ticket for that?

atobiszei · 2025-04-22T09:53:13Z

src/test/pull_hf_model_test.cpp

+    std::unique_ptr<std::thread> t;
+
+    void ServerPullHfModel(std::string& sourceModel, std::string& repoPath) {
+        ::SetUpServerForDownload(this->t, this->server, sourceModel, repoPath);


We don't need to start whole server to verify HfDownloader

We can use the class functionality but I wanted to have integration tests -> end to end with small model.
This can be late reused to test final solution.

atobiszei · 2025-04-22T09:56:03Z

src/libgt2/libgt2.cpp

+    return hfEndpoint;
+}
+
+std::string HfDownloader::GetRepoUrl(std::string& hfEndpoint) {


Add tests for creating url based on various TOKEN, ENDPOINTS. Add negative tests

michalkulakowski · 2025-04-22T14:53:12Z

ci/build_test_OnCommit.groovy

@@ -82,6 +82,7 @@ pipeline {
              when { expression { image_build_needed == "true" } }
                steps {
                      sh "echo build --remote_cache=${env.OVMS_BAZEL_REMOTE_CACHE_URL} > .user.bazelrc"
+                      sh "echo test:linux --test_env https_proxy=${env.HTTPS_PROXY} > .user.bazelrc"


Wouldn't this overwrite effect of the command above? ">>" probably should be used

atobiszei · 2025-04-23T11:18:02Z

src/cli_parser.cpp

@@ -26,6 +26,7 @@
 #include <ntstatus.h>
 #endif
 #include "capi_frontend/server_settings.hpp"
+#include "libgt2/libgt2.hpp"


atobiszei · 2025-04-23T11:18:12Z

src/BUILD

@@ -340,6 +340,7 @@ cc_library(
        "@com_github_jarro2783_cxxopts//:cxxopts",
        "libovms_server_settings",
        "libovms_version",
+        "//src/libgt2:libgt2",


dependency leakage

atobiszei · 2025-04-23T11:18:38Z

src/BUILD

@@ -549,6 +550,8 @@ cc_library(
        "get_model_metadata_impl.hpp",
        "global_sequences_viewer.hpp",
        "global_sequences_viewer.cpp",
+        "hf_pull_model_module.hpp",


separate target

atobiszei · 2025-04-23T11:19:42Z

src/config.cpp

@@ -93,6 +93,10 @@ bool Config::check_hostname_or_ip(const std::string& input) {
 }

 bool Config::validate() {
+    // TODO: Add validation of all parameters once the CLI model export flags will be implemented


This TODO should be part of a ticket.

atobiszei · 2025-04-23T11:21:26Z

src/hf_pull_model_module.cpp

+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//*****************************************************************************


shouldn't those two files be in libgt2 dir?

I can move it there.

atobiszei · 2025-04-23T11:22:09Z

src/libgt2/libgt2.cpp

+}
+
+bool HfDownloader::CheckIfProxySet() {
+    const char* envCred = std::getenv("https_proxy");


We still do have get env in HfDownloader

Removed today.

atobiszei · 2025-04-23T11:43:38Z

src/libgt2/libgt2.cpp

+    if (envCred) {
+        hfEndpoint = std::string(envCred);
+    } else {
+        SPDLOG_DEBUG("HF_ENDPOINT environment variable not set");


Is this reason to stop OVMS if we require to pull but endpoint is not set?

We use default endpoint huggingface.co

atobiszei · 2025-04-23T11:48:27Z

src/test/pull_hf_model_test.cpp

+
+    ASSERT_EQ(hfDownloader->GetRepoUrl(hfEndpoint), "https://www.new_hf.com/model/name");
+
+    ASSERT_EQ(hfDownloader->GetRepositoryUrlWithPassword(hfEndpoint), "https://123$$o_O123!AAbb:[email protected]_hf.com/model/name");


Approach with envs with leak env in case of failura at asserts. We could have instead
ASSERT_EQ(HfDownloader(modelName, downloadPath, false, hfEndpoint, hf_token, useProxy).GetRepositoryUrlWithPassword(), "https://123$$o_O123!AAbb:[email protected]_hf.com/model/name");

Then you could easily add additional lines for testing incorrect downloadPaths vaildation, invalid modelName convetions (special characters?), endpoint not being proper url, etc. instead of having setup envs separaterly each time.

Added task for additional validation of variables from the user.

atobiszei · 2025-04-24T14:19:44Z

src/libgt2/libgt2.hpp

+    std::string hfEndpoint;
+    std::string hfToken;
+    std::string httpProxy;
+    bool pullHfModelMode;


Why is this here? Either we create module for pulling or not. I don't see a point.

Left from previous implementation. Good catch to remove.

atobiszei · 2025-04-24T14:24:38Z

src/test/pull_hf_model_test.cpp

+    ASSERT_EQ(hfDownloader->getEndpoint(), "www.new_hf.com/");
+    ASSERT_EQ(hfDownloader->GetRepoUrl(), "https://www.new_hf.com/model/name");


Those are verified in GetRepositoryUrlWithPassword. We don't need to test if setting those values in constructor works.

Only checking for proxy makes sense but even then it is better to document what is considered proxy not set for hf downloader with
EXPECT_EQ(TestHfDownloader(modelName, downloadPath, false, hfEndpoint, hfToken, "").isProxySet(), false)
or
EXPECT_EQ(TestHfDownloader(modelName, downloadPath, false, hfEndpoint, hfToken, "").isProxySet(), false)

This is standard unit test approach to test all methods of a class. Specifically when one method returns values that are used in another method. I can change the EXPECT_EQ for proxy though for better readability.

.

src/libgt2/libgt2.cpp

Co-authored-by: Adrian Tobiszewski <[email protected]>

rasapala added 12 commits March 4, 2025 16:22

Cmake builds

09ae985

Init missing

66fb2cf

Working with proxy

02727f6

Style

1a4d718

Fix chineses file by removing all files

7fdcd5c

Style

baf7344

Fix so dependency

e561904

Merge branch 'main' into CVS-163308_libgt2

22692c6

License

a580a43

Spell in license

211fc40

Fix redhat

982ab97

Merge remote-tracking branch 'origin/CVS-163308_libgt2' into CVS-1633…

dc673d5

…08_libgt2

atobiszei reviewed Mar 7, 2025

View reviewed changes

third_party/libgt2/libgt2_engine.bzl Outdated Show resolved Hide resolved

rasapala added 13 commits March 7, 2025 14:41

Workong HF_TOKEN

86f9c31

Does not work lfs

a8de843

Working fiters

cb0e77f

Add file name

c0fbfdf

Merge branch 'main' into CVS-163308_libgt2

baaf77c

Linux compile

44c2787

Set workdir on windows

90c26b3

Working linux

8b3d09c

Merge branch 'main' into CVS-163308_libgt2

fe6b425

Add credentails to url

21005f2

Merge branch 'CVS-163308_libgt2' of https://github.com/openvinotoolki…

bb4d800

…t/model_server into CVS-163308_libgt2

WIP

34835af

Merge remote-tracking branch 'origin/CVS-163308_libgt2' into CVS-1633…

fb499e1

…08_libgt2

rasapala changed the title ~~Cmake builds libgit2~~ [WIP] Cmake builds libgit2 Mar 19, 2025

rasapala added 2 commits March 19, 2025 14:30

Pass, hf_endpoint and rewrite

7c5e798

Merge branch 'main' into CVS-163308_libgt2

b1e1790

rasapala added 6 commits April 17, 2025 11:08

Print info changes

9bc522b

Change to INFO

00d693e

Fix proxy and debug

e825390

No script

b38520f

Take proxy from build configuration

bad937d

Add noproxy

bb4bdd2

dtrawins reviewed Apr 22, 2025

View reviewed changes

atobiszei previously approved these changes Apr 22, 2025

View reviewed changes

atobiszei self-requested a review April 22, 2025 10:05

Merge branch 'main' into CVS-163308_libgt2

17f3d9d

michalkulakowski reviewed Apr 22, 2025

View reviewed changes

rasapala added 4 commits April 22, 2025 17:31

Code review and tests

1489c16

Fix append

b6d1627

Fix setenv for windows

187953e

Style

374e925

atobiszei reviewed Apr 23, 2025

View reviewed changes

atobiszei self-requested a review April 23, 2025 12:17

rasapala added 3 commits April 24, 2025 13:47

Env to module

8b03342

Code review

69620fa

Merge branch 'main' into CVS-163308_libgt2

424b992

atobiszei reviewed Apr 24, 2025

View reviewed changes

atobiszei self-requested a review April 24, 2025 14:27

Code review

1ca8e79

atobiszei reviewed Apr 25, 2025

View reviewed changes

src/libgt2/libgt2.cpp Outdated Show resolved Hide resolved

atobiszei approved these changes Apr 25, 2025

View reviewed changes

rasapala and others added 2 commits April 25, 2025 13:15

Update src/libgt2/libgt2.cpp

5a889b7

Co-authored-by: Adrian Tobiszewski <[email protected]>

Pragma

ce23428

michalkulakowski approved these changes Apr 25, 2025

View reviewed changes

rasapala merged commit 4b7a96e into main Apr 25, 2025
12 checks passed

		@@ -0,0 +1,78 @@
		//***************************************************************************
		// Copyright 2022 Intel Corporation


		ASSERT_EQ(hfDownloader->GetRepoUrl(hfEndpoint), "https://www.new_hf.com/model/name");

		ASSERT_EQ(hfDownloader->GetRepositoryUrlWithPassword(hfEndpoint), "https://123$$o_O123!AAbb:[email protected]_hf.com/model/name");

		ASSERT_EQ(hfDownloader->getEndpoint(), "www.new_hf.com/");
		ASSERT_EQ(hfDownloader->GetRepoUrl(), "https://www.new_hf.com/model/name");

Cmake builds libgit2 and --pull_hf_mode for ovms #3105

Cmake builds libgit2 and --pull_hf_mode for ovms #3105

Uh oh!

Conversation

rasapala commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛠 Summary

🧪 Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rasapala commented Mar 4, 2025 •

edited

Loading

rasapala Apr 24, 2025 •

edited

Loading