common : add minimalist multi-thread progress bar

[angt]

I intentionally kept the bar simple without specifying part numbers (which ultimately don't matter much) the only thing we care about is tracking progress

[ngxson]

Unless I missed something, I can't find any references to std::thread in download.cpp - do we actually using more than 1 thread here?

And AFAIK, httplib::Client is single-threaded.

Edit: nevermind, see next comment

[ngxson]

This looks like a local variable at first glance, but because it's static, that effectively make it a global variable.

IMO we should avoid global variable if possible. Probably a better way is to make a dedicated class to manage threads and implement the mutex as a member of the class.

[ngxson]

Hmm, nevermind, I found it: the multi-threads is launched by std::async which uses a thread pool under the hood.

In this case, I think it's better to introduce a mutex at common_download_file_multiple level (not global/static level)

This is the code where threads are created:

    std::vector<std::future<bool>> futures_download;
    for (auto const & item : urls) {
        futures_download.push_back(std::async(std::launch::async, [bearer_token, offline](const std::pair<std::string, std::string> & it) -> bool {
            return common_download_file_single(it.first, it.second, bearer_token, offline);
        }, item));
    }

The cleaner idea looks like this (pseudo-code):

    std::mutex mutex_progress;
    std::vector<float> progress(urls.size(), 0); // from 0.0 to 100.0
    auto update_progress = [&mutex_progress, &progress](size_t index, float value) {
        std::lock_guard<std::mutex> lock(mutex_progress);
        progress[index] = value;
        // print all progress here
    };

    std::vector<std::future<bool>> futures_download;
    for (auto const & item : urls) {
        futures_download.push_back(std::async(std::launch::async, [bearer_token, offline, update_progress](const std::pair<std::string, std::string> & it) -> bool {
            return common_download_file_single(it.first, it.second, bearer_token, offline, update_progress);
        }, item));
    }

[angt]

The cleaner idea looks like this (pseudo-code):

    std::mutex mutex_progress;
    std::vector<float> progress(urls.size(), 0); // from 0.0 to 100.0
    auto update_progress = [&mutex_progress, &progress](size_t index, float value) {
        std::lock_guard<std::mutex> lock(mutex_progress);
        progress[index] = value;
        // print all progress here
    };

    std::vector<std::future<bool>> futures_download;
    for (auto const & item : urls) {
        futures_download.push_back(std::async(std::launch::async, [bearer_token, offline, update_progress](const std::pair<std::string, std::string> & it) -> bool {
            return common_download_file_single(it.first, it.second, bearer_token, offline, update_progress);
        }, item));
    }

I disagree on that, moving the lock to the caller is error-prone. Statics are generally avoided for good reasons, but in this case it fits perfectly as there is physically only one terminal to share.

The only bad thing is the static map, but it keeps the code extremely simple and works for the current usage (avoiding the need to change all prototypes). When wefully remove curl, we can refactor the entire download code for better structure and address this properly.

[angt]

@ngxson If you prefer, we can wait for #17427 to be merged. I can then propose a more intrusive refactor with a proper class and RAII to handle the map cleanup

[ngxson]

I disagree on that, moving the lock to the caller is error-prone.

Which kind of error can we get?

Statics are generally avoided for good reasons, but in this case it fits perfectly as there is physically only one terminal to share.

You can't assume this function will always be called in terminal application unless you explicit specify it in the function name or documentation.

Currently, there is nothing prevent someone from calling common_download_file_multiple from one of the tool/example. When server implement the /download endpoint to add model dynamically from the webui, without seeing this discussion, I will end up calling it from multiple thread, and didn't expect it to have a global mutex. This is a real example of why your code can be error-prone.

---

Edit: the given example does not directly related to the placement of mutex, but I just want to highlight the fact that functions should be well-documented especially in term of thread safety

[angt]

Which kind of error can we get?

We need a function to print the progress bar, as it could be called from many places. By moving the mutex outside of the function, you are literally asking for a non thread-safe function to be synchronized by the caller, which is error-prone by design. As this function shares a unique tty for the whole lifetime of the process, a static mutex is the perfect scenario here. There are no instances to handle.

You can't assume this function will always be called in terminal application unless you explicit specify it in the function name or documentation.

The function is designed to be safe regardless of the context, whether it's running in a terminal or not.

Currently, there is nothing prevent someone from calling common_download_file_multiple from one of the tool/example. When server implement the /download endpoint to add model dynamically from the webui, without seeing this discussion, I will end up calling it from multiple thread, and didn't expect it to have a global mutex. This is a real example of why your code can be error-prone.

It will work in that case as well, you just call the function from anywhere. If multiple threads call it, you will see multiple progress bars, which is the expected behavior :)

[angt]

Since #17427 is merged, I'll update the implementation to properly cleanup the map in all cases.

[ngxson]

It will work in that case as well, you just call the function from anywhere. If multiple threads call it, you will see multiple progress bars, which is the expected behavior :)

In server's use case, I would expect to have an abstracted implementation of the progress bar (whose the state can be reported into webui). Each progress instance of the download should have their own progress bar and they none depends on the other.

Probably this implementation is OK for just terminal and we can improve it later. But still, having a static mutex inside a function is obviously an anti-pattern (it should ideally in global scope)

Probably better to move it to log.cpp and reuse the already existing common_log::mtx (ofc, we also need to initialize log system prior to model downloading, which is currently not the case - but this is trivial to do)

[angt]

Totally agree, for the WebUI we'll need a better abstraction of the downloader, to completly remove all the tty code.

Probably better to move it to log.cpp and reuse the already existing common_log::mtx (ofc, we also need to initialize log system prior to model downloading, which is currently not the case - but this is trivial to do)

That wouldn't be good. The progress bar needs a tty fd, while the log goes to stderr (for a modern process) or a dedicated fd. They shouldn't be mixed (you don't want a progress bar in your logs anyway).

[angt]

The new implementation works like the old one but cleanup the map in all cases thanks to RAII. Also, instead of the thread id, we now use the instance itself to track the bar, which is more flexible.

[ngxson]

That wouldn't be good. The progress bar needs a tty fd, while the log goes to stderr (for a modern process) or a dedicated fd. They shouldn't be mixed (you don't want a progress bar in your logs anyway).

I don't mean the progress bar must use the same tty as log, but it should use the same mutex as log. In theory, that should be the only mutex controlling output to console/tty/file

[angt]

That wouldn't be good. The progress bar needs a tty fd, while the log goes to stderr (for a modern process) or a dedicated fd. They shouldn't be mixed (you don't want a progress bar in your logs anyway).

I don't mean the progress bar must use the same tty as log, but it should use the same mutex as log. In theory, that should be the only mutex controlling output to console/tty/file

Actually, this is a known anti-pattern we need to avoid. Since the download is a long process exposed to interrupts (signals, network loss, disk full...) sharing the mutex creates a guaranteed deadlock if we need to log something.

The mutex is absolutely not here to protect the fd, it's purely cosmetic to ensure a clean display when everything goes well as we share the same tty.

[ngxson]

I cannot think of any cases that it can result in a deadlock:

• The progress bar only acquires a lock if it wants to display something, then release it
• The log system also acquires a lock if it wants to display something:
  • If the lock cannot be acquired, there is another on-going log line or a progress bar on-going. In this case, we wait until the log line or progress bar finishes its work

At least, that's how I envision the system to work.

Also, I think we try to reduce the number of std::mutex across the code base whenever possible. Probably @ggerganov can you give some opinions here?

[ngxson]

Since the download is a long process exposed to interrupts (signals, network loss, disk full...)

Just to note that, download is a long process, but print_progress is not - it is more or less an enhanced version of LOG(...) with the ability to update the last line.

Edit: even better, I think we can just implement a new log macro: LOG_PROG(...), meaning "log using progress bar"