New apis for downloading #2695

JohanMabille · 2023-07-21T11:40:41Z

No description provided.

Klaim

Good job!

Some missing things

this really needs to be tested in the c++ tests as once plugged it will be a fundamental piece of the whole library. I understand it is not easy to test because it's using curl internally and this cannot be replaced by some networking fake api, but maybe we can at least test with files in local directory?
documentation of the types and functions apis in core/download.hpp

Klaim · 2023-07-21T11:51:13Z

libmamba/include/mamba/core/context.hpp

@@ -134,6 +134,8 @@ namespace mamba
            int retry_timeout{ 2 };  // seconds
            int retry_backoff{ 3 };  // retry_timeout * retry_backoff
            int max_retries{ 3 };    // max number of retries
+
+            std::map<std::string, std::string> proxy_servers;


Did you consider std::unordered_map here?

I moved this member from somewhere else, changing the type for an unordered_map requires many changes in the configuration, let's do this in a dedicated PR to unordered_map.

Klaim · 2023-07-21T11:52:15Z

libmamba/include/mamba/core/download.hpp

+{
+    struct TransferData
+    {
+        int http_status;


Klaim · 2023-07-21T11:52:32Z

libmamba/include/mamba/core/download.hpp

+
+    struct DownloadProgress
+    {
+        std::size_t downloaded_size;


= 0; ? and for the following member

Klaim · 2023-07-21T11:54:11Z

libmamba/include/mamba/core/download.hpp

+
+    struct DownloadOptions
+    {
+        bool fail_fast;


please provide default values 🥲

Klaim · 2023-07-21T11:54:45Z

libmamba/include/mamba/core/download.hpp

+
+    MultiDownloadResult download(MultiDownloadRequest requests,
+                                 const Context& context,
+                                 DownloadOptions options);


Consider setting DownloadOptions options = {} for quick default usage?

Klaim · 2023-07-21T12:25:20Z

libmamba/src/core/download.cpp

+    void Downloader::prepare_next_downloads()
+    {
+        size_t running_attempts = m_completion_map.size();
+        size_t max_parallel_downloads = p_context->threads_params.download_threads;


Klaim · 2023-07-21T12:26:34Z

libmamba/src/core/download.cpp

+            if (!msg.m_transfer_done)
+            {
+                // We are only interested in messages about finished transfers
+                continue;


does pop_message() handles the waiting of the current thread? I would use std::yield() here otherwise, to avoid saturating the cpu.

nope this should be done in m_curl_handle->perform() actually, which is missing from this function.

Klaim · 2023-07-21T12:29:09Z

libmamba/src/core/download.cpp

+
+    MultiDownloadResult Downloader::build_result() const
+    {
+        DownloadResultList l;


please avoid 1 letters or abreviations names in non-maths code🥲

Klaim · 2023-07-21T12:29:50Z

libmamba/src/core/download_impl.hpp

+        using on_success_callback = std::function<bool(DownloadSuccess)>;
+        using on_failure_callback = std::function<bool(DownloadError)>;
+
+        DownloadAttempt(CURLHandle& handle, const DownloadRequest* request);


As pointed before, request should be taken by const reference, passing nullptr should not be authorized.

Klaim · 2023-07-21T12:30:19Z

libmamba/src/core/download_impl.hpp

+        using completion_function = DownloadAttempt::completion_function;
+        using completion_map_entry = std::pair<CURLId, completion_function>;
+
+        DownloadTracker(const DownloadRequest* request, std::size_t max_retries, bool fail_fast);


same remark about request should be a const ref here

Also to avoid the bool in the interface of DownloadTRacker, consider grouping both value arguments in a DownloadTrackerOptions struct with them and default values.

Actually I did it in the first version but I found it a bit overkilled for only two members. I will reintroduce it, I agree the boolean here is kind of ugly.

Hind-M · 2023-08-10T08:12:15Z

libmambapy/src/main.cpp

@@ -578,6 +577,7 @@ PYBIND11_MODULE(bindings, m)
        .def_readwrite("retry_backoff", &Context::RemoteFetchParams::retry_backoff)
        .def_readwrite("user_agent", &Context::RemoteFetchParams::user_agent)
        // .def_readwrite("read_timeout_secs", &Context::RemoteFetchParams::read_timeout_secs)
+        .def_readwrite("proxy_servers", &Context::RemoteFetchParams::proxy_servers)


Btw, if we still want backward compatibility with python API, we need to add something like this:
https://github.com/mamba-org/mamba/blob/main/libmambapy/src/main.cpp#L605-L621

wolfv · 2023-08-23T07:16:06Z

.github/workflows/main.yml

@@ -188,6 +188,7 @@ jobs:
        shell: bash -l {0}
        run: |
          pybind11-stubgen libmambapy.bindings
+          pre-commit run --files stubs/libmambapy/binginds-stubds/__init__.pyi


Is this a typo? binginds-stubds -> bindings-stubs?

Yes definitely! Thanks for catching it ;)

* New apis for downloading * Changes according to review * Added minimal test and fixed issues * Python backward compatibility

Klaim reviewed Jul 21, 2023

View reviewed changes

JohanMabille force-pushed the download branch 7 times, most recently from e569e3c to af525f8 Compare July 28, 2023 14:08

JohanMabille marked this pull request as ready for review July 28, 2023 14:09

JohanMabille force-pushed the download branch 2 times, most recently from 86ee5d8 to 100efba Compare July 28, 2023 14:14

Hind-M reviewed Aug 10, 2023

View reviewed changes

JohanMabille added 3 commits August 22, 2023 09:12

New apis for downloading

c119178

Changes according to review

7c8bf68

Added minimal test and fixed issues

d9f0f2e

JohanMabille force-pushed the download branch 4 times, most recently from 58a5ba9 to 27dc742 Compare August 22, 2023 13:46

Python backward compatibility

726916c

JohanMabille force-pushed the download branch from 27dc742 to 726916c Compare August 22, 2023 14:04

JohanMabille merged commit 6e4d91e into mamba-org:main Aug 23, 2023
29 checks passed

JohanMabille deleted the download branch August 23, 2023 07:07

wolfv reviewed Aug 23, 2023

View reviewed changes

cvanelteren pushed a commit to cvanelteren/mamba that referenced this pull request Aug 29, 2023

New apis for downloading (mamba-org#2695)

57e6d20

* New apis for downloading * Changes according to review * Added minimal test and fixed issues * Python backward compatibility

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New apis for downloading #2695

New apis for downloading #2695

JohanMabille commented Jul 21, 2023

Klaim left a comment

Klaim Jul 21, 2023

JohanMabille Jul 21, 2023 •

edited

Klaim Jul 21, 2023

Klaim Jul 21, 2023

Klaim Jul 21, 2023

Klaim Jul 21, 2023

Klaim Jul 21, 2023

Klaim Jul 21, 2023

JohanMabille Jul 21, 2023

Klaim Jul 21, 2023

Klaim Jul 21, 2023

Klaim Jul 21, 2023

Klaim Jul 21, 2023

JohanMabille Jul 21, 2023

Hind-M Aug 10, 2023

wolfv Aug 23, 2023

JohanMabille Aug 23, 2023 •

edited

New apis for downloading #2695

New apis for downloading #2695

Conversation

JohanMabille commented Jul 21, 2023

Klaim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanMabille Jul 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanMabille Aug 23, 2023 • edited

Choose a reason for hiding this comment

JohanMabille Jul 21, 2023 •

edited

JohanMabille Aug 23, 2023 •

edited