-
Notifications
You must be signed in to change notification settings - Fork 13.1k
common : use cpp-httplib as a cURL alternative for downloads #16185
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
base: master
Are you sure you want to change the base?
common : use cpp-httplib as a cURL alternative for downloads #16185
Conversation
Signed-off-by: Adrien Gallouët <[email protected]>
ecd182e
to
0201e99
Compare
The existing cURL implementation is intentionally left untouched to prevent any regressions and to allow for safe, side-by-side testing by toggling the `LLAMA_CURL` CMake option. Signed-off-by: Adrien Gallouët <[email protected]>
0201e99
to
e1f545f
Compare
Signed-off-by: Adrien Gallouët <[email protected]>
Signed-off-by: Adrien Gallouët <[email protected]>
This is the one that concerns me, since
|
It shouldn't be too difficult to add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. The biggest unknown for me is the Windows workflow - building and releases. I suppose whatever we currently do to provide libcurl
we have to do for libssl
.
If you plan to bring this to completion, feel free to add yourself to the CODEOWNERS. I see the following TODOs:
- Extract file downloading implementation from
common/arg.cpp
tocommon/download.cpp
- Remove CURL dependency (+ figure out how to build on Windows)
- Remove json dependency from
common/download.cpp
- Add CMake option to build without
httplib
for old Windows support
static void write_metadata(const std::string & path, | ||
const std::string & url, | ||
const common_file_metadata & metadata) { | ||
nlohmann::json metadata_json = { | ||
{ "url", url }, | ||
{ "etag", metadata.etag }, | ||
{ "lastModified", metadata.last_modified } | ||
}; | ||
|
||
write_file(path, metadata_json.dump(4)); | ||
LOG_DBG("%s: file metadata saved: %s\n", __func__, path.c_str()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as in the previous PR about the json stuff: I hope eventually we will avoid using json for this component - it's a pity we started doing it in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely can remove json here, in fact we can just read/write the etag it's enough.
Signed-off-by: Adrien Gallouët <[email protected]>
The windows issue comes from updating Possible solutions could be either patching |
Signed-off-by: Adrien Gallouët <[email protected]>
Yes, we should stick with the latest version of
The idea is when I suspect that these failing CI workflows are currently happening only for the |
Regarding 547fa26 - I suppose this is temporary? We want to keep the upstream version unchanged, so any modifications should be first upstreamed to the original repo. |
Yes, this was only to confirm that everything builds correctly with it. |
Since |
Oh right, I missed that when I wrote the comment earlier.
Yes, let's give this a try. |
…rwin Signed-off-by: Adrien Gallouët <[email protected]>
1f97bec
to
aad19ef
Compare
Note @ggerganov : I've tested the version including commit 547fa26 (cpp-httplib: allow _WIN32_WINNT >= 0x0602) and it works fine under Wine. There should be no issue retargeting Windows 8 if needed.
And no issues when linking libssl statically on Windows :)
|
Signed-off-by: Adrien Gallouët <[email protected]>
Signed-off-by: Adrien Gallouët <[email protected]>
aad19ef
to
e7b5f55
Compare
Hm, the address sanitizer is acting up. Not sure if related though. |
I think the binaries were compiled with flags that are not supported by the emulator. All the errors are
I guess it’s just random hardware selection. I can dig into that later. |
I restarted the workflows. If CI is green, I think we are good to merge, correct? |
Yes! I can refactor (downloader.cpp) and remove json just after |
Yup, this should be fixed before merging. |
I cleared the ccache cache of the sanitizer test before you re-ran the CI, I suspect that was the cause. |
Signed-off-by: Adrien Gallouët <[email protected]>
.github/workflows/build.yml
Outdated
-DCMAKE_SYSTEM_NAME=Linux \ | ||
-DGGML_CCACHE=OFF \ | ||
-DGGML_NATIVE=OFF \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm don't think I understand how this change fixed the CI for ubuntu-cpu-make
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit extreme on this one and tried everything I could think of to make it work.
The tricky part is CMAKE_SYSTEM_NAME=Linux
, which makes CMake believe we are cross-compiling (setting CMAKE_CROSSCOMPILING
) but without breaking everything else. With that flag, i can disable GGML_NATIVE_DEFAULT
:
Lines 98 to 103 in 835b2b9
if (CMAKE_CROSSCOMPILING OR DEFINED ENV{SOURCE_DATE_EPOCH}) | |
message(STATUS "Setting GGML_NATIVE_DEFAULT to OFF") | |
set(GGML_NATIVE_DEFAULT OFF) | |
else() | |
set(GGML_NATIVE_DEFAULT ON) | |
endif() |
then disabling GGML_NATIVE
allows us not to set INS_ENB
:
Lines 134 to 138 in 835b2b9
if (GGML_NATIVE OR NOT GGML_NATIVE_DEFAULT) | |
set(INS_ENB OFF) | |
else() | |
set(INS_ENB ON) | |
endif() |
This way we get the lowest CPU mode. But honestly, -DGGML_NATIVE=OFF
should be enough, and I also disabled ccache
to increase my chances :)
Signed-off-by: Adrien Gallouët <[email protected]>
This is a draft that uses httplib to download, mostly copied from the existing cURL implementation.
To test, build with
-DLLAMA_CURL=OFF
.Some features might be missing for now, but it's a starting point.