Skip to content
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

✨ Progress bar, reformatting #32

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dario-loi
Copy link

✨ Progress Bars! ✨

As requested by #17, a number of progress bars have been added to the script thanks to the library tqdm, the progress bars show current progres during lengthy operations such as mod downloading and linkage.

All print statements that are contained inside of a tqdm context (basically a for loop that is being tracked by a progress bar) have been rewritten to use tqdm.tqdm.write, as to not break the progress bars, tqdm.tqdm.write prints stuff above the progressbar, so the bar always stays at the bottom, for example:

downloading_mods

📝 Formatting 📝

All the scripts have been formatted by the black formatter for improved readability (admittedly this is kind of intrusive and I should have separated this on a second commit, shame on me).

📌 Requirements 📌

Generated a requirements.txt listing the minimal requirements in order to run the scripts (by creating a venv, installing requests and tqdm and then running pip freeze > requirements.txt.

Now users can install all dependencies in one go as follows:

git clone https://github.com/cdbbnnyCode/modpack-installer.git
cd modpack-installer
pip install -r ./requirements.txt

@dario-loi
Copy link
Author

If this is merged into main, please remember to re-add the API key, I removed it from my fork.

@cdbbnnyCode
Copy link
Owner

Hi, thanks for the PR! A couple thoughts:

  • Thanks for the reformat! I usually wouldn't say that, but I did go back to change some logic in the previous release and noticed that 2021 me was not very good at python, haha. That said, I'm still not going to be continuously auto-formatting everything.
  • I'm not sure if it's worth requiring a second (fairly large) dependency just for progress bars, but I'll consider it.

@dario-loi
Copy link
Author

Hi! terribly sorry for not separating the formatting and the actual changes, if that was the case we could've at least integrated the formatting right away.

On the subject of the dependency, I used tqdm as it is the de-facto standard for progress bars and it's quite easy to integrate (just wrap the iterators with a function), you can probably find a lighter alternative but then again it probably won't have asyncio support out of the box.

Finally, as a comment, the usage of asyncio is kind of outdated (I don't think one needs to call the event loop manually nowadays), but would there really be a point in refactoring now that it works?

@cdbbnnyCode
Copy link
Owner

Yup, the asyncio is not actually used anymore (note that the thread pool executor only has one thread). That needs to be cleaned up, I'll probably do that pretty soon.

@dario-loi
Copy link
Author

Out of curiosity, why did you choose to not use asyncio anymore?

@cdbbnnyCode
Copy link
Owner

I'm trying to be super conservative about how many requests the program makes, since the CF page (when I looked at it last) said they might revoke API keys and/or add charges for excessive amounts of requests, but they never specified how many requests that might be. It's probably not that big of a deal, but I'd rather not have to worry about it.

Previously, the API had no key or other usage tracking (as far as I know), so this program would use 4 different asyncio threads to pull in mods simultaneously as fast as the server/network could send them. Once the new API rolled around, I just changed the thread count to 1, effectively removing the actual functionality of the asyncio part without having to change anything.

The main reason for rate-limiting so heavily is that each mod download (even if the file already exists) has to make 2 JSON requests to find the actual download location and file type. I'm not an expert in webserver stuff, but that seems like it adds up to a lot of requests pretty quickly.

@dario-loi
Copy link
Author

That is understandable, however, is there no way to know if the mod has been downloaded already? I see the mods are already stored in a modcache folder, perhaps the pair (projectID, fileID) could be used to build some sort of map in a file that can be loaded and checked before sending requests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants